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

Allow to supress warnings for sanity checks #359

Merged

Conversation

RolandStuder
Copy link
Contributor

Enhancement

Description

Changes confg.exit_on_failed_sanity_checks that uses a boolean, to config.on_failed_sanity_checks with three options :exit, :warn, :ignore

Some people were seeing the warning on test runs, which can be annoying especially if you are working with TDD. So it would be better to provide an opt out possibility.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing
  • This is not a documentation update

@RolandStuder
Copy link
Contributor Author

@leastbad
Copy link
Contributor

One thing that is self-evident from the conversations with both AbuMareBear and Dave Sanders is that there absolutely needs to be a section in the documentation on setting up the Rails test environment so that it can work with the same general configuration used in development. Any actual testing, whether with minitest, RSpec or "does the witch float" could follow.

I am concerned that we are optimizing to minimize the annoyance of folks who (innocently and benevolently) haven't set up their test environments. It stands to reason that if they follow some basic, one-time steps, they won't be plagued with errors. Right?

  1. Make sure you're running Redis
  2. Configure Gemfile with redis and hiredis
  3. Configure test environment of cable.yml similar to development
  4. Update test.rb environment to set cache_store and enable action controller caching
  5. Make sure redis server connection details are available via env or credentials

ANYHOW. 😀

I really like the implementation you've come up with. I could be crazy but what you call :warn could be anything that isn't :ignore or :exit, right? Like literally any other value?

I'm glad that we're still defaulting to :exit, which I read as "warn, then exit" - right?

I am going to make a tiny non-functional tweak to the error message to point to the correct location.

@RolandStuder
Copy link
Contributor Author

RolandStuder commented Nov 11, 2020

I really like the implementation you've come up with. I could be crazy but what you call :warn could be anything that isn't :ignore or :exit, right? Like literally any other value?

Would be fine for me, but you never know what the future holds. I almost thought to treat everything but :exit and :warn as :ignore, as this would have allowed for this: .config.on_sanity_checks = :crickets :-)

@leastbad
Copy link
Contributor

What do you think? Too much?

@RolandStuder
Copy link
Contributor Author

@leastbad Nah, that's nice to offer such clear information. I like it.

Copy link
Contributor

@hopsoft hopsoft left a comment

Choose a reason for hiding this comment

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

This looks great. I had one minor nitpick.

@hopsoft hopsoft merged commit da7f8b5 into stimulusreflex:master Nov 11, 2020
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