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

Logging only in fit mode #5711

Closed
brigand opened this issue Mar 3, 2018 · 15 comments · Fixed by #13700
Closed

Logging only in fit mode #5711

brigand opened this issue Mar 3, 2018 · 15 comments · Fixed by #13700

Comments

@brigand
Copy link
Contributor

brigand commented Mar 3, 2018

Do you want to request a feature or report a bug?

Feature

I often add debug logging in my programs, which I'd like to see in one specific case: fit() (focus-it) annotated tests, but I usually end up doing something like if (global.JEST_IS_FIT) console.log(x) in my source code, and global.JEST_IS_FIT = true in my test, and then remove it from the test when I delete the 'f' in 'fit'.

The code in the source code isn't that bad, but in the test file I'd still like to enable the logging by adding a character or two. Also, it'd be good to have a consistent convention and tooling for this in all projects using jest.

I don't have an exact proposal for how this should look in the source and test file. Just some ideas:

  • dit instead of fit, for 'debug-it'
  • console.debug only shows up in this mode; both in the test file and source code
  • process.env.JEST_FIT or similar. For code going through webpack, DefinePlugin could be used to cause DCE

Concerns:

  • I would like this to work without restarting jest (e.g. no a flag/env var)
  • It seems cleaner to tie this to the test file instead of a hotkey in the interactive watch mode because you want to both focus the test and enable debug logging in one operation, and then remove both in one operation.
  • The code under test might call other functions that also use the debug logging mechanism
@SimenB
Copy link
Member

SimenB commented Mar 5, 2018

Not sure I understand. If you have a focused test, only that is run. Why isn't that enough? Could you add a more concrete example of your use case?

@brigand
Copy link
Contributor Author

brigand commented Mar 5, 2018

Sure, so at the time I posted this, I was writing an argument parser, and I have this in the source:

    if (global.JEST_IS_FIT) {
      console.log(
        `Debug at arg ${i} of "${arg}"`,
        `\nState:`,
        state,
        `\nResult:`,
        result,
      );
    }

I have a bunch of its in the test. When one would fail, I'd change it to fit, then add a line that does global.JEST_IS_FIT = true to enable the debug logging, fix the issue, remove the 'f' in 'fit', remove the global.JEST_IS_FIT = true, and then move on.

I can't leave it on consistently because you don't usually want logs in tests. I don't want to remove the code from the source and add it back every time, so it needs to be a conditional. I want a way to more easily enable the debug logging when I need to.

I wouldn't ever enable the debug logging if multiple tests were running, because it'd be way too much noise to actually see the relevant information, which is why this specifically applies to fit.

Another case I have is a babel plugin with a bunch of test cases, and there's debug logging that's only really useful when debugging a specific test. In the babel plugin, there are cases where an operation is skipped because it's not supported, but I'd like to know it's being skipped when debugging a failing test.

This really applies to any time you're debugging one failing test, and the logging is valuable enough to keep in the code long-term.

This being part of jest would mean someone working on either of these could know the proposed convention in jest, and just enable it while they're debugging a test. It wouldn't rely on me expressing some convention specific to a given project.

@rickhanlonii
Copy link
Member

I can see exposing the force test mode status so that anyone (including custom setup/reporters/watchers/etc) can create a custom behavior for force mode if they want to

If we exposed it, then you could accomplish what you want with:

console.debug = message => {
  if (global.isForcedMode) {  // however it's exposed
    console.log(messag);
  }
}

@brigand
Copy link
Contributor Author

brigand commented Mar 7, 2018

That would work well for me, good idea.

@SimenB
Copy link
Member

SimenB commented Feb 15, 2019

We should add if a test was focused to the test result. At that point, this should be trivial to implement in a custom matcher. We currently track if it passed or failed, but we should add a field for skipped and focused as well.

https://github.com/facebook/jest/blob/d9d501ac342212b8a58ddb23a31518beb7b56f47/types/TestResult.js#L183-L192


Just doing this in Circus should be fine, fwiw

@mackness
Copy link
Contributor

mackness commented Mar 6, 2019

Hi @SimenB, do you mind if I pick this one up?

@SimenB
Copy link
Member

SimenB commented Mar 6, 2019

Yeah, go for it! Note that this is just adding it to the test result, not changing the reporter :)

@mattphillips
Copy link
Contributor

Is this something that we actually want in Jest? It seems like the problem here is trying to leave long living debugs using console.log.

This problem can be solved by using a logger that supports levels as an environment variable.

Given that we are trying to adopt: https://fishshell.com/docs/current/design.html, I'm not sure it is worth adding this to core.

I don't think we should encourage people to add blocks of debug code to application code with Jest internals (especially if they are long living!) or to override console.debug just for tests. Both of these options will increase application code with potentially unwanted side effects, i.e. leaving a console.debug in by mistake.

Instead we should recommend using a logger with levels so that devs can confidently add logs to their code and just set the level relevant to the environment the code is being ran in.

Thoughts @SimenB @rickhanlonii?

@SimenB
Copy link
Member

SimenB commented Mar 14, 2019

What I think we should do is add to TestResult that a test was skipped/focused etc. Once that's in, a custom reporter can decide to omit console.log from any test not focused.

Makes sense? I do not think we should add anything to the global environment that user code can use to decide whether to log or not

@github-actions
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Feb 25, 2022
@ghost
Copy link

ghost commented Mar 24, 2022

hi @SimenB , i see this got stale label. is someone working on it?
if not, can i work on it?

@github-actions github-actions bot removed the Stale label Mar 24, 2022
@grazirs
Copy link

grazirs commented Aug 25, 2022

Hi @SimenB! I'd like to work on this issue by applying your suggestion, but I'm wondering how to know if the test is being focused inside the formatTestResult function, I saw there is the 'skipped' status that I can access with: testResult.skipped but I did not find anything related to 'focused', is it available somewhere else?

@hkaur008
Copy link

is this issue open to work ?

@SimenB
Copy link
Member

SimenB commented Jan 24, 2023

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants