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

Ensure service is reset after scenario test failures #175

Merged
merged 1 commit into from
May 17, 2024

Conversation

akouryy
Copy link
Contributor

@akouryy akouryy commented May 17, 2024

This PR fixes a bug in scenario_compiler.rb where core.reset! is not called when a test case fails. Due to this issue, one failing scenario can result in verbose failure messages that indicate irrelevant scenarios have also failed.

It seems that scenario_compiler.rb itself currently does not have test cases. While I have not added any in this PR, I will give it a try if requested.

Reproduction procedure

  • Add scenario/a1.rb with the following content.
## update: test1.rb
f("a")

## assert
This is a wrong assertion.
  • Add scenario/a2.rb with the following content.
## update: test2.rb
def f(x) = nil
f(0)

## assert
class Object
  def f: (Integer) -> nil
end
  • Run rake test.

Expected Behavior

Only a1.rb fails.

Actual Behavior (prior to this PR)

a2.rb also fails, due to the code introduced by a1.rb.

Failure: test: scenario/a2.rb(ScenarioCompiler::ScenarioTest)
scenario/a2.rb:5:in `block in <class:ScenarioTest>'
<"class Object\n" + "  def f: (Integer) -> nil\n" + "end"> expected but was
<"class Object\n" + "  def f: (Integer | String) -> nil\n" + "end">

@mame mame merged commit f44aac3 into ruby:v2 May 17, 2024
6 checks passed
@mame
Copy link
Member

mame commented May 17, 2024

Amazing! I've seen this problem many times, but never figured out how to fix it. Thank you!

@akouryy akouryy deleted the ensure-reset-in-scenario-test branch May 18, 2024 00:44
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