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

Include ActiveSupport::Testing::TaggedLogging for Rails >= 7 (rebased) #2587

Merged
merged 2 commits into from
Sep 9, 2022

Conversation

JonRowe
Copy link
Member

@JonRowe JonRowe commented Apr 3, 2022

This is #2580 rebased

@JonRowe JonRowe mentioned this pull request Apr 3, 2022
21 tasks
@pirj
Copy link
Member

pirj commented Apr 11, 2022

The failure is due to this line calling name. I recall we have had and had fixed a similar problem not long ago.

@pirj pirj force-pushed the shanecav84/include-tagged-logging branch from 66a0be0 to a34dea7 Compare April 11, 2022 18:07
@pirj
Copy link
Member

pirj commented Apr 11, 2022

Green.
I have mixed feeling from defining the name method for all Rails example groups. A spec with let(:name) works as expected.
Wondering what other side effects might be.

@JonRowe
Copy link
Member Author

JonRowe commented Apr 12, 2022

I have mixed feeling from defining the name method for all Rails example groups. A spec with let(:name) works as expected.
Wondering what other side effects might be.

A let(:name) { .. } would override the definition here, and thus if it was not string safe it likely would break... but... this is the case regardless...

@pirj
Copy link
Member

pirj commented Apr 12, 2022

I see what you mean. With the var naming, though, name, chances of it not having a to_s are lower.
Still, it would be a weird side effect to show it in tagged logging output.
As far as I understand it, name should evaluate to the example title.
Any idea why it is not yet available in this context?
I tried with RSpec.current_example.metadata[:name], it was empty too.
In any case, @example resolves to nil. And it's not because it's explicitly nullified in the constructor, it is nil if you set a breakpoint above, too.

As an alternative approach with no side effects, we would have to wrap tagger_logger, delegate to it, and provide it some name. In the best case, our example name. Or just nil if it turns out to be impossible.
I can take a stab at it if you think it's a viable approach.

@pirj
Copy link
Member

pirj commented Apr 15, 2022

Unfortunately enough, name is used not only in before_setup, but in assertions, too, e.g. this:

        def _assert_nothing_raised_or_warn(assertion, &block)
          assert_nothing_raised(&block)
        rescue Minitest::UnexpectedError => e
          if tagged_logger && tagged_logger.warn?  warning = <<~MSG
              #{self.class} - #{name}: #{e.error.class} raised.

I'll push the wrapped that I've written, but it fails.

@pirj
Copy link
Member

pirj commented Jul 17, 2022

I'd go with a risk of name raising when its definition is incapable of .to_s, and it logging nonsense when name is defined.
@JonRowe Any objections on merging as is?

@pirj pirj force-pushed the shanecav84/include-tagged-logging branch from 0096e1b to 38a3675 Compare July 19, 2022 19:26
RSpec.current_example.metadata[:name] evaluates to `nil` by the time
this is called.
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