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

New: Improve debugging for RuleTester #67

Closed

Conversation

CoryDanielson
Copy link

@CoryDanielson CoryDanielson commented Sep 21, 2020

Summary

Add a before hook to the valid/invalid code configuration, so that developers can inject a debugger into RuleTester before code is passed into the rule being tested.

Related Issues

Open issue in eslint/eslint: eslint/eslint#13625

@nzakas nzakas added Initial Commenting This RFC is in the initial feedback stage and removed triage labels Sep 22, 2020
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up. I feel like this is really a proposal to figure out a way to test just one snippet of code easily and less about using the debugger statement.

For instance, if we added a only: true option to each code block you care about and skipped the rest, similar to Mocha’s it.only, would that solve the problem?

1. Add an optional `before` function property to the config for valid/invalid code.
2. Update RuleTester to call this function (if it exists) just prior to the valid/invalid code being passed into the rule.
3. If the autofix code is tested as well, the before hook should be called before that as well.
4. Because of the potential to call the `before` hook twice - it may make sense to pass in an argument that defines when this code is being called. (ie a string: 'create' | 'fix') This would enable a developer to conditionally debug whichever code they want.
Copy link
Member

Choose a reason for hiding this comment

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

This seems very unexpected. Can you explain why it would be run twice?

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking that the before hook might be called before RuleTester passes code into the create and fix methods for an ESLint rule. I might be mistaken about how that works, because I haven't written a rule that implements fix yet.


## Summary

Add a `before` hook to the valid/invalid code configuration, so that developers can inject a debugger into RuleTester before code is passed into the rule being tested.
Copy link
Member

Choose a reason for hiding this comment

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

I’m a bit worried at how focused this is on the debugger case. Are there any other potential uses?

Copy link
Author

Choose a reason for hiding this comment

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

I couldn't immediately think of another use case that wasn't already solved by another API. I thought before and after would allow for performance timing, but ESLint has documentation about performance analysis on rules.

@CoryDanielson
Copy link
Author

CoryDanielson commented Sep 22, 2020

Thanks for writing this up. I feel like this is really a proposal to figure out a way to test just one snippet of code easily and less about using the debugger statement.

For instance, if we added a only: true option to each code block you care about and skipped the rest, similar to Mocha’s it.only, would that solve the problem?

My concern with this was that by testing a single code snippit, you don't know if you've broken other tests. I bumped into this problem exactly while writing a rule. I was developing against a single code snippit, got it passing, then uncommented the others and saw that my changes had broken a handful of others.

it.only would be an improvement over the current scenario but using it would still be a less-than-ideal workflow when debugging

@nzakas
Copy link
Member

nzakas commented Sep 23, 2020

I guess I’m used to the it.only process of getting that test working and then checking to see if any other tests broke, so that doesn’t bother me too much.

I’m not sold on this proposal as of this moment, but let’s see what others think.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

I've done a similar workflow in the past of faking .only by commenting out all the other cases. Having an API to debug one case while still running all the others without debugging would be ideal in a vacuum, though implementing that might be pretty hairy because of the way RuleTester works. If a before hook is indeed too complex, I'd be open to considering a .only equivalent since it'd still be an improvement over the status quo.

## Detailed Design

1. Add an optional `before` function property to the config for valid/invalid code.
2. Update RuleTester to call this function (if it exists) just prior to the valid/invalid code being passed into the rule.
Copy link
Member

Choose a reason for hiding this comment

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

RuleTester doesn't call rules directly. It calls linter.verify(...). If RuleTester were to call before immediately before the verify() call, it would still take developers on a tour of ESLint internals before they get to their rule code.

Copy link
Author

Choose a reason for hiding this comment

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

That might make this a non-starter. A way to avoid stepping through the internals would be if the rule was conditionally edited and a debugger was inserted into it by RuleTester

@nzakas nzakas added tsc agenda Topic for discussion at next TSC meeting and removed tsc agenda Topic for discussion at next TSC meeting labels Nov 3, 2020
@CoryDanielson
Copy link
Author

CoryDanielson commented Feb 5, 2021

Rejected in favor of #73

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Initial Commenting This RFC is in the initial feedback stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants