Skip to content

Conversation

@sfc-gh-kbregula
Copy link
Contributor

@sfc-gh-kbregula sfc-gh-kbregula commented Apr 12, 2023

📚 Context

This file contained tests for many different unrelated components. I have now moved the tests to files consistent with what module is being tested.

This is not related to any project, but I decided to clean it up because due to conflicts in this file, I had to rebase more than once.

Please describe the project or issue background here

  • What kind of change does this PR introduce?

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

🧠 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.

It is duplicated in lib/tests/streamlit/elements/legacy_altair_test.py line 41
It is duplicated by lib/tests/streamlit/elements/arrow_altair_test.py line 44
It is duplicated by lib/tests/streamlit/elements/legacy_altair_test.py line 125
It is duplicated by lib/tests/streamlit/elements/arrow_altair_test.py line test_arrow_chart_with_x_y
It is duplicated in lib/tests/streamlit/elements/arrow_altair_test.py line 292
It is duplicated by lib/tests/streamlit/elements/arrow_dataframe_test.py line 51
@sfc-gh-kbregula sfc-gh-kbregula marked this pull request as ready for review April 12, 2023 09:08
@sfc-gh-kbregula sfc-gh-kbregula added the security-assessment-completed Security assessment has been completed for PR label Apr 18, 2023
Copy link
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

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

This seems to me like a reasonable and good restructuring 👍 I'm not sure about the actual reason why all those elements got mixed into a single file. I assume it only grew this way historically, but maybe there is another reason here. Probably best to do another quick check with @tconkling before merging.

@tconkling
Copy link
Contributor

This is a good change - and yeah, streamlit_test.py is an artifact of the oldest days of Streamlit (and it makes sense to refactor)!

@sfc-gh-kbregula sfc-gh-kbregula merged commit d67f0dd into develop Apr 20, 2023
tconkling added a commit to tconkling/streamlit that referenced this pull request Apr 20, 2023
* develop:
  Dont use constraints file in scheduled builds (streamlit#6470)
  Reorganize tests from streamlit_test.py (streamlit#6481)
  Date and Time test classes (streamlit#6501)
@sfc-gh-kmcgrady sfc-gh-kmcgrady deleted the sfc-gh-kbregula-clean-up-tests branch October 5, 2023 19:30
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.

5 participants