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

Handle gracefully fixtures that only exist for tests in TestHelper.unload_fixture #72

Conversation

serioushaircut
Copy link
Contributor

Description

When using the test helper with fixtures that only exist under their name in tests, then unload_fixture was bailing as it tried to force load those model classes from their "original" path.

What approach

Just rescue from any StandardError (load records throws Errno::ENOENT) on load_records in the unload_fixture implementation.

What else was considered

Leaving all test frozenrecords with names that also exist in the normal path and splitting them by different path names.
That was a bit unwieldy to have so many directories names like tests which all contained the same file name.

Questions

  • Is their another way to do that and not blowing up the path structure for multiple fixtures?
  • Should we log maybe?
  • Should I catch the Errno::ENOENT specifically?

begin
model_class.load_records(force: true)
rescue
nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than swallow the error it should be re-raised.

Passing a wrong path shouldn't silently fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the point: there isn't a "wrong" path.

Look, the fixture and frozen record only exists for the test, and can hence not be forcefully loaded on 'unload'

"Nothing to go back to" in that case

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see. Sorry I misunderstood your PR description.

Then this should be detected as part of load and registered somewhere so unload knows not to call load_records.

Rescueing an exception like this could hide a legitimate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k, something like:
during load_fixture if the model exists under the alternate path but not under the original/configured path => store it a bit differently (or just not putting it into the @cache)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if the model exists under the alternate path but not under the original/configured path => store it a bit differently (or just not putting it into the @cache)?

Yes. e.g. store false or some other token value you can check in unload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

voted for putting nil into the cache and only force-loading the original backend file when the cache has a non nil value. I think that expresses the idea quite well - there isn't a basepath to go back to, so it's nil. okay?

@serioushaircut serioushaircut force-pushed the frozen-test-helper-unload-fixture-gracefully branch from 324e3f9 to fcf1814 Compare February 19, 2024 17:24
@serioushaircut serioushaircut force-pushed the frozen-test-helper-unload-fixture-gracefully branch from fcf1814 to 8e15b87 Compare February 19, 2024 17:30
Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
@serioushaircut
Copy link
Contributor Author

TIL 3.2 got rid of exists?

@byroot byroot merged commit 513d757 into byroot:master Feb 19, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants