Skip to content

Conversation

@halfdan
Copy link
Contributor

@halfdan halfdan commented Mar 2, 2023

Addresses https://groups.google.com/g/elixir-lang-core/c/yjs-zaFvZJc

Looking for feedback on implementation and testing strategy.

  • Update docs for seed argument

Comment on lines +398 to +399
options = persist_defaults(configuration())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to be outside of Task.async to ensure seed was set when it's fetched in test.ex

assert matches == [["ATest [test/a_test_stale.exs]"], ["BTest [test/b_test_stale.exs]"]]
end)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Those tests are rather expensive and they rely on implementation details-ish. I am thinking we could just skip them altogether? Other than that, the impl is great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me! Let me remove the test.

@halfdan
Copy link
Contributor Author

halfdan commented Mar 3, 2023

@josevalim I considered updating the docs for the --seed argument, but now I'm thinking that they are probably fine as is:

    * `--seed` - seeds the random number generator used to randomize the order of tests;
      `--seed 0` disables randomization so the tests in a single file will always be ran
      in the same order they were defined in

Even with --seed 0 there's no guarantee the modules are loaded in the same order given the parallelisation in how the modules are loaded.

@josevalim josevalim merged commit 31b3a20 into elixir-lang:main Mar 3, 2023
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@halfdan halfdan deleted the fb/shuffle-require branch March 3, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants