Skip to content

Conversation

@jcoyne
Copy link
Member

@jcoyne jcoyne commented Oct 15, 2024

No description provided.

@jcoyne jcoyne force-pushed the fix-assets branch 3 times, most recently from f39e1cc to 9a6d0a1 Compare October 15, 2024 03:51
@jcoyne jcoyne changed the base branch from empty_commit to main October 15, 2024 04:25
within_test_app do
# Precompiles the javascript
system "bin/rake spec:prepare"
end
Copy link
Member

Choose a reason for hiding this comment

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

In my work in blacklight_range_limit, I solved this like this instead:

https://github.com/projectblacklight/blacklight_range_limit/blob/da22926ef99ac2e1d3cb04a750a57ea0ed08b3a8/Rakefile#L14-L17

I think spec:prepare ought to do it, rather than a custom blacklight:build_host_assets? I am wary of adding additional complexity here, without understanding why the existing logic wasn't working, and don't understand what this change is meant to accomplish -- in both cases "spec:prepare" is being run before the specs are run, what is expected to be the difference here?

I also wonder if we coudl replace that system shell-out with a simple rake dependency to existing spec:prepare like demo'd in blacklight_range_limit -- I have become acutely aware of how long rake ci takes to run, as I try to work with it in an iterative loop, and a shell out to bin/rspec, with an additional app boot time added in, is definitely adding some time.

Copy link
Member Author

@jcoyne jcoyne Oct 15, 2024

Choose a reason for hiding this comment

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

Doesn't this have to happen within the test app? If you just call spec:prepare won't that run at the engine root?

Isn't range limit also creating a custom task to do the same? https://github.com/projectblacklight/blacklight_range_limit/blob/da22926ef99ac2e1d3cb04a750a57ea0ed08b3a8/Rakefile#L37-L45

Copy link
Member

@jrochkind jrochkind Oct 15, 2024

Choose a reason for hiding this comment

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

Oh nevermind, you are absolutely right, I did similar to what you are doing here, yup.

Still not sure what this change would change, since within-test-app spec-prepare should have been called before tests were run already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This just moves it closer to running the tests. It was something that worked on my local machine to fix the failures for certain seeds. I'm not sure why it doesn't work on CI.

@jcoyne jcoyne force-pushed the fix-assets branch 5 times, most recently from 40dfaf5 to da9288c Compare October 15, 2024 15:52
@jcoyne jcoyne added this to the 9.X milestone Oct 16, 2024
@jrochkind
Copy link
Member

Has this ended up no superceded by other PR's, once discovered that it was a devise bug? I am not totally keeping up with them all, sorry.

@jrochkind
Copy link
Member

i think this may be no longer needed?

@jcoyne
Copy link
Member Author

jcoyne commented Nov 6, 2024

I still prefer breaking this out into a discreet task. It's just a refactor of existing functionality.

@jcoyne jcoyne merged commit 6d27db8 into main Jan 21, 2025
11 checks passed
@jcoyne jcoyne deleted the fix-assets branch January 21, 2025 15:43
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.

5 participants