connorshea / vscode-ruby-test-adapter

A Ruby test adapter extension for the VS Code Test Explorer
https://marketplace.visualstudio.com/items?itemName=connorshea.vscode-ruby-test-adapter
MIT License
83 stars 50 forks source link

Set Minitest.seed when needed #100

Closed blowmage closed 1 year ago

blowmage commented 2 years ago

Ensure a seed is present when running Minitest tests.

[fixes #99]

pnomolos commented 1 year ago

This needs to be updated to the following (I shortened the syntax, but the important bit is the root-level module reference):

::Minitest.seed ||= (ENV["SEED"] || srand).to_i % 0xFFFF if ::Minitest.respond_to?(:seed)

Otherwise it just doesn't work, because VSCode::Minitest get resolved before Minitest and it doesn't have a seed method, so the conditional fails.

blowmage commented 1 year ago

Updated.

SergeyBurtsev commented 1 year ago

Just a thought: maybe we can define the sorted order for tests instead of random order when building the list before calling #runnable_methods and thereby remove the dependency on seed completely? And then we don't need to sort the tests by their line numbers afterwards. What do you think?

def build_list
  tests = []
  ::Minitest::Runnable.runnables.map do |runnable|
    runnable.define_singleton_method(:test_order) { :sorted }
    file_tests = runnable.runnable_methods.map do |test_name|
      ...
    end
    # file_tests.sort_by! { |t| t[:line_number] }
    ...
  end
end
pnomolos commented 1 year ago

The whole point of ordering a test suite randomly is to discover issues caused by hidden dependencies or a shared mutable state due to tests being run in a certain order, so my vote for that would be no.

SergeyBurtsev commented 1 year ago

Good point @pnomolos . Thought, maybe build_list method used for building the list of tests only and running those tests uses different flow, but I double checked and see, that build_list is used for running tests as well, so tweaking test_order for runnables is not good.

blowmage commented 1 year ago

Ping. Anything else needed to get this change accepted?

connorshea commented 1 year ago

Nope, change seems reasonable to me. I'll run CI and assuming that passes, I'll merge and cut a new release.

connorshea commented 1 year ago

Spoken too soon :( It looks like the tests are failing on main as well, though, so let me see if I can figure that out.

connorshea commented 1 year ago

...so I guess this changed solved a secondary problem in CI, so now CI passes. Neat, didn't expect that. Thank you!

blowmage commented 1 year ago

Thanks!

finnove commented 1 year ago

Great! Thank you! This seem to have solved my problem in https://github.com/minitest/minitest/issues/917 (running minitest 5.16.3 this time, but I think this seed fix was the crucial one 👍)