Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Unit tests by moving random seeds to setUp #2267

Closed
wants to merge 2 commits into from

Conversation

krishnakalyan3
Copy link
Contributor

Following up on #2033.

Copy link
Member

@nateanl nateanl left a comment

Choose a reason for hiding this comment

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

Hi @krishnakalyan3, thanks for the PR! Some test methods still use seed when calling get_whitenoise. For example, the methods under datasets test. Could you also address them in this PR?

@krishnakalyan3
Copy link
Contributor Author

@nateanl looks like the seed is being changed in all dataset tests. Do we modify the random seed within the loop for all datasets?.

@mthrok
Copy link
Collaborator

mthrok commented Mar 6, 2022

Hi @krishnakalyan3, thanks for the PR! Some test methods still use seed when calling get_whitenoise. For example, the methods under datasets test. Could you also address them in this PR?

@zhangguanheng66 This is intended design. The problem that the original PR was solving is automatically setting the seed of the library code that is run. If seed is not reset at the start of every test run, the test results end up depend on the order the test is executed. Test utilities allowing explicit seed is the right thing to do, otherwise it will implicitly depend on the global seed state, which could interfere the library code behavior.

@krishnakalyan3 the initial state of the PR looked fine. Can you revert the latest commit? (git revert @ on your branch should do) We can merge it and follow up for the remaining.

facebook-github-bot pushed a commit that referenced this pull request Jun 1, 2022
Summary:
Bringing in move seed commit from previous open commit #2267. Organizes seed to utils.

Pull Request resolved: #2425

Reviewed By: carolineechen, nateanl

Differential Revision: D36787599

Pulled By: skim0514

fbshipit-source-id: 37a0d632d13d4336a830c4b98bdb04828ed88c20
facebook-github-bot pushed a commit that referenced this pull request Jun 3, 2022
Summary:
For test files where applicable, removed manual seeds where applicable. Refactoring #2267

Pull Request resolved: #2436

Reviewed By: carolineechen

Differential Revision: D36896854

Pulled By: skim0514

fbshipit-source-id: 7b4dd8a8dbfbef271f5cc56564dc83a760407e6c
@carolineechen
Copy link
Contributor

completed in #2425 and #2436

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants