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

Use remove_const after specs which set constants #1138

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MaxLap
Copy link
Contributor

@MaxLap MaxLap commented Feb 27, 2024

So I'm making a tool which runs specs many times, and specs that set consts seems to often not cleanup after themselves. This makes the repeat have annoying warnings about already defined consts.

Those constants existing between specs also seems like something that could create an interaction between them.

So I added the remove_const calls that make sense to avoid all of these warnings. Good news, no tests relied on that!

@eregon
Copy link
Member

eregon commented Feb 29, 2024

This makes the repeat have annoying warnings about already defined consts.

What about just ignoring those warnings for that use case?
For example by running specs with $VERBOSE = nil/RUBYOPT=-W0?

Another way would be to define Warning.warn, MSpec used to do that but it can cause problems: ruby/mspec@1d8cf64.
So I don't want to reintroduce that, or at least not by default.

I have 2 concerns with this change:

  • It seems very likely to regress so we would need a CI check for this.
  • It looks quite messy with these ~130 added remove_const. There are already some remove_const (68) but IIRC those are necessary to run specs with repeat > 1 (some specs would fail otherwise).

@eregon
Copy link
Member

eregon commented Feb 29, 2024

Redefining constants seems rather harmless in the context of MSpec, when it's not autoload constants.
OTOH it's true it's global state that "leaks" between repeated runs of the same specs (different spec examples should use different constants so there is no concern without --repeat). But then the spec on the first run does pass without previous state, so it feels rather harmless again.

@MaxLap
Copy link
Contributor Author

MaxLap commented Feb 29, 2024

For many cases, you are right that just ignore warnings would probably have been fine.

For some cases, such as when doing include and extend, then with enough repetition of a new module, you get non-linear timings and increasing memory usage. For these, ignoring the warnings is not possible for my use case.

I understand your concerns, and I had them and wondered if you would accept the changes as I was doing them. The way I see it:

  • Regression are not a problem for the current normal usage of this tool. This is just some work that was done, which make things cleaner and more reusable by others. I don't think a CI check is needed for this, at least not yet. My goal is not to add more work on maintainers. There will be regression, and someone that needs the remove_const to be right will clean them up next time it's needed, but at least he won't have to repeat the cleanups that I did.
  • It's messy when you look only at those 271 lines of code. I feel that the remove_const actually make the individual test clearer as that they highlight what global mutation where actually done. Especially for some of those subtle cases that are changing the singleton_class.

My question to you is, do you think the cleanups should be in ensure or not? If the test fails before the constant is defined, then the ensure will also fail. Not sure how the error message would look then. At the same time, the ensure highlight that it's not part of the test, but that may also be what make it feel noisy when you focus on it.

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