Skip to content

Conversation

@kajarenc
Copy link
Collaborator

@kajarenc kajarenc commented Dec 16, 2022

📚 Context

Previous Cypress Tests implementation sometimes (depending on a number of e2e specs) not running tests at the end of the list of tests. E.g. cypress22 job on the first commit of this PR not runs e2e/specs/typography.spec.js, e2e/specs/widget_state_heavy_usage.spec.js specs.

This PR changes the logic of calculation for a number of specs per worker, and should always run all e2e tests regardless of the number of tests.

Based on the previous version if we have 135 test: the jobs will be divided equally between 22 workers by 135 / 21 = 6 test spec per job. And 6 * 22 = 132 will be not running.

In the new implementation, we calculate the remainder remainder=total_number_of_spec/22 and assign the first #reminder workers per one extra test spec.

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe:

🧠 Description of Changes

  • Add bullet points summarizing your changes here

    • This is a breaking API change
    • This is a visible (user-facing) change

Revised:

Insert screenshot of your updated UI/code here

Current:

Insert screenshot of existing UI/code here

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests 😅

🌐 References

Does this depend on other work, documents, or tickets?

  • Issue: Closes #XXXX

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@kajarenc kajarenc added the security-assessment-completed Security assessment has been completed for PR label Dec 16, 2022
@kajarenc kajarenc changed the title Empty commit to run CI Change workers balancing logic for e2e tests to always cover all specs Dec 16, 2022
@kajarenc kajarenc marked this pull request as ready for review December 16, 2022 20:30
Copy link
Collaborator

@mayagbarnes mayagbarnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you 🎉

@kajarenc kajarenc merged commit e678004 into streamlit:develop Dec 18, 2022
tconkling added a commit that referenced this pull request Dec 20, 2022
* develop:
  Change workers balancing logic for e2e tests to always cover all specs (#5865)
  Up version to 1.16.0 (#5852)
  Stop installing pip-tools (#5854)
  Add eslint no relative import paths plugin to pre commit hooks (#5839)
  Docs: move note to `experimental_allow_widgets` param description (#5843)
  Update element docstrings for colored text support (#5831)
  Add remark-directive plugin (Support for colored text in markdown) (#5774)
  Teach WebsocketConnection how to wait on an external auth token (#5728)
  Turn on streamlit theme for altair and plotly (#5796)
  Fix markdown headers spacing (#5829)
  Update error message and docstring in st.map (#5792)
  Remove unnecessarily methods from DeltaGenerator (#5824)
  Resolve vega-tooltip to later version (#5825)
  Easter Egg (#5817)
  Update Exception Layout to avoid overflow (#5700)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants