Fix sync-tag eslint rule #647
Conversation
🦋 Changeset detectedLatest commit: 434c5f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
| @@ -1,6 +1,7 @@ | |||
| const {execFile} = require("child_process"); | |||
| const {execFile, execSync} = require("child_process"); | |||
There was a problem hiding this comment.
It seems like using types would've solved this specific issue, as well, right?
There was a problem hiding this comment.
That too. This code needs a lot of TLC.
|
Size Change: 0 B Total Size: 4.42 kB ℹ️ View Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #647 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 93 93
Lines 1352 1349 -3
Branches 343 328 -15
=========================================
- Hits 1352 1349 -3 see 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Summary:
The tests didn't catch this because they were mocking it. The reason why we need to bother with these util wrappers around functions is that RuleTester doesn't provide a way to mock things so I ended up having to mock things in a hacky way. I added an assert() call to the place where we do the mocking, but I think we need rethink how we write eslint tests. We should probably create our own jest-based test harness, but that'll take some doing so I'm going to punt on it for now.
Issue: None
Test plan: