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

test: properly verify dev server exec #379

Merged
merged 1 commit into from
Nov 23, 2023
Merged

test: properly verify dev server exec #379

merged 1 commit into from
Nov 23, 2023

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Nov 17, 2023

Summary

Currently some of the tests are not being run as their assertion code just isn't being called - from what I can tell, I don't think RSpec has ever supported passing a block to receive in a way that gets executed (except maybe if the mocked method actually expects a block but I've not tested that).

Pull Request checklist

Remove this line after checking all the items here. If the item does not apply to the PR, both check it out and wrap it by ~.

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

Other Information

This does reveal some interesting behaviour with the https logic that is probably worth addressing:

  1. determining if https is being used is based on checking the protocol but there's an https? method too - it looks like this generally just has not been deprecated properly
  2. the runner errors if --https is provided but https isn't enabled but it doesn't do the inverse check; it also seems like the runner could automatically provide --https and so just encourage setting server: "https" in the config?
    • I've not done a lot with --https and co, so there might be some valid reason for why Shakapacker requires the user to provide both
  3. the runner exits using exit! which means those branches can't be easily tested - ideally it would be best to throw an error that could be caught, but even just switching to exit will make it easier to test (as under the hood that throws SystemExit which you can expect to receive); I would argue that's better anyway for a library because there might be cleanup hooks registered downstream that should be called

@G-Rath
Copy link
Contributor Author

G-Rath commented Nov 17, 2023

@ahangarha would you mind reviewing this and merging if you've happy since CI is green and it'll let @SimenB add tests for #378

@ahangarha
Copy link
Contributor

why Shakapacker requires the user to provide both

We prefer the server: "https" but still support the old configuration for backward compatibility.

I think it is a good idea to plan for deprecation and eventually removal of the old config for the next major release.

@justin808 Do you agree?

Copy link
Contributor

@ahangarha ahangarha left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏾

@ahangarha ahangarha requested a review from Judahmeek November 21, 2023 12:20
@justin808 justin808 merged commit e272496 into shakacode:master Nov 23, 2023
41 checks passed
@G-Rath G-Rath deleted the fix-tests branch November 23, 2023 18:56
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.

3 participants