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.

Sign up for GitHub

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

Jump to bottom

chore: error on unused eslint disables #26510

Merged
merged 1 commit into from
Aug 17, 2023
Merged

chore: error on unused eslint disables #26510

merged 1 commit into from
Aug 17, 2023

Conversation

mastrzyz
Copy link
Contributor

What?

Fail the CI/Local build if we are using eslint-disable spuriously .

More information -> here

Why?

It can be confusing seeing an eslint-disable in code and there is no actual violation. Like found here :

export const webServer = (options: WebServerPluginOptions): TestRunnerPlugin => {
  // eslint-disable-next-line no-console
  return new WebServerPlugin(options, false);
};

This most likely was a re-factoring or debt reduction which the author forgot to remove the eslint-disable statement.

@github-actions
Copy link
Contributor

Test results for "tests 1"

3 failed
❌ [webkit] › library/trace-viewer.spec.ts:131:1 › should render network bars
❌ [playwright-test] › playwright.trace.spec.ts:353:5 › should retain traces for interrupted tests
❌ [playwright-test] › playwright.unhandled.spec.ts:36:5 › should lead in unhandledRejection when page.route raises

12 flaky
⚠️ [chromium] › library/tracing.spec.ts:239:5 › should not include trace resources from the previous chunks
⚠️ [chromium] › library/tracing.spec.ts:239:5 › should not include trace resources from the previous chunks
⚠️ [chromium] › library/tracing.spec.ts:239:5 › should not include trace resources from the previous chunks
⚠️ [firefox] › page/page-event-request.spec.ts:162:3 › should return response body when Cross-Origin-Opener-Policy is set
⚠️ [firefox] › page/workers.spec.ts:103:3 › should clear upon navigation
⚠️ [playwright-test] › ui-mode-test-progress.spec.ts:167:5 › should update tracing network live
⚠️ [playwright-test] › ui-mode-trace.spec.ts:53:5 › should merge web assertion events
⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all
⚠️ [webkit] › library/browsercontext-reuse.spec.ts:50:1 › should reset serviceworker
⚠️ [playwright-test] › ui-mode-test-progress.spec.ts:218:5 › should show trace w/ multiple contexts
⚠️ [playwright-test] › ui-mode-trace.spec.ts:22:5 › should merge trace events
⚠️ [playwright-test] › ui-mode-trace.spec.ts:53:5 › should merge web assertion events

25034 passed, 583 skipped
✔️✔️✔️

❌ [webkit] › library/trace-viewer.spec.ts:131:1 › should render network bars 

Error: Timed out 10000ms waiting for expect(received).toHaveCount(expected) // deep equality

Expected: 1
Received: 0
Call log:
  - expect.toHaveCount with timeout 10000ms
  - waiting for locator('.timeline-bar.network')
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"


  133 |     await page.goto(server.EMPTY_PAGE);
  134 |   });
> 135 |   await expect(traceViewer.page.locator('.timeline-bar.network')).toHaveCount(1);
      |                                                                   ^
  136 | });
  137 |
  138 | test('should render console', async ({ showTraceViewer, browserName }) => {

    at /home/runner/work/playwright/playwright/tests/library/trace-viewer.spec.ts:135:67

Retry 1:

Error: Timed out 10000ms waiting for expect(received).toHaveCount(expected) // deep equality

Expected: 1
Received: 0
Call log:
  - expect.toHaveCount with timeout 10000ms
  - waiting for locator('.timeline-bar.network')
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"


  133 |     await page.goto(server.EMPTY_PAGE);
  134 |   });
> 135 |   await expect(traceViewer.page.locator('.timeline-bar.network')).toHaveCount(1);
      |                                                                   ^
  136 | });
  137 |
  138 | test('should render console', async ({ showTraceViewer, browserName }) => {

    at /home/runner/work/playwright/playwright/tests/library/trace-viewer.spec.ts:135:67

Retry 2:

Error: Timed out 10000ms waiting for expect(received).toHaveCount(expected) // deep equality

Expected: 1
Received: 0
Call log:
  - expect.toHaveCount with timeout 10000ms
  - waiting for locator('.timeline-bar.network')
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"


  133 |     await page.goto(server.EMPTY_PAGE);
  134 |   });
> 135 |   await expect(traceViewer.page.locator('.timeline-bar.network')).toHaveCount(1);
      |                                                                   ^
  136 | });
  137 |
  138 | test('should render console', async ({ showTraceViewer, browserName }) => {

    at /home/runner/work/playwright/playwright/tests/library/trace-viewer.spec.ts:135:67

Retry 3:

Error: Timed out 10000ms waiting for expect(received).toHaveCount(expected) // deep equality

Expected: 1
Received: 0
Call log:
  - expect.toHaveCount with timeout 10000ms
  - waiting for locator('.timeline-bar.network')
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"
  -   locator resolved to 0 elements
  -   unexpected value "0"


  133 |     await page.goto(server.EMPTY_PAGE);
  134 |   });
> 135 |   await expect(traceViewer.page.locator('.timeline-bar.network')).toHaveCount(1);
      |                                                                   ^
  136 | });
  137 |
  138 | test('should render console', async ({ showTraceViewer, browserName }) => {

    at /home/runner/work/playwright/playwright/tests/library/trace-viewer.spec.ts:135:67

❌ [playwright-test] › playwright.trace.spec.ts:353:5 › should retain traces for interrupted tests 

Test timeout of 30000ms exceeded.

❌ [playwright-test] › playwright.unhandled.spec.ts:36:5 › should lead in unhandledRejection when page.route raises 

Test timeout of 30000ms exceeded.

Merge workflow run.

mxschmitt
mxschmitt reviewed Aug 17, 2023
View reviewed changes
Copy link
Member

@mxschmitt mxschmitt left a comment

Choose a reason for hiding this comment

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

Why not put it in the ESLint config instead?

reportUnusedDisableDirectives

@mastrzyz
Copy link
Contributor Author

mastrzyz commented Aug 17, 2023 •
edited
Loading

Why not put it in the ESLint config instead?

reportUnusedDisableDirectives

@mxschmitt That's a possibility but there is a very weird discrepancy in ESLint, if you use

  • reportUnusedDisableDirectives:true you get a warning
/Users/marcinstrzyz/src/playwrightfix/packages/playwright-test/src/reporters/list.ts
  17:1  warning  Unused eslint-disable directive (no problems were reported from 'no-console')
  • --report-unused-disable-directives you get an error
/Users/marcinstrzyz/src/playwrightfix/packages/playwright-test/src/reporters/list.ts
  17:1  error  Unused eslint-disable directive (no problems were reported from 'no-console')

@mxschmitt mxschmitt merged commit 6b2d93a into microsoft:main Aug 17, 2023
@commit-lint commit-lint bot mentioned this pull request Aug 17, 2023
Merged
Germandrummer92 pushed a commit to OctoMind-dev/playwright that referenced this pull request Oct 27, 2023
@mastrzyz @Germandrummer92
chore: error on unused eslint disables (microsoft#26510)
48ad011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxschmitt mxschmitt mxschmitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mastrzyz @mxschmitt