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

Add --fix-to-stdout back #307

Merged
merged 4 commits into from
Aug 2, 2024
Merged

Add --fix-to-stdout back #307

merged 4 commits into from
Aug 2, 2024

Conversation

mantoni
Copy link
Owner

@mantoni mantoni commented Aug 1, 2024

This includes the work from @DamienCassou made in #298 with more unit tests and documentation.

Fixes #295

@mantoni mantoni force-pushed the add-fix-to-stdout-back branch 3 times, most recently from 16fe56d to b34fea1 Compare August 1, 2024 21:21
@mantoni
Copy link
Owner Author

mantoni commented Aug 1, 2024

This is very frustrating. It's always the same integration test failing, always on some other version of eslint, and I cannot reproduce locally.

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 1, 2024

and I cannot reproduce locally.

I think you can try clean linux environment? It's reproducible on my machine - this test fails with random eslint version

my setup is:
Ubuntu 24.04 server minimal
nodejs 22 installed with nvm

then I just cloned this branch, run npm ci, then npm test:integration -- --timeout 10000

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 1, 2024

This is what I get from stderr:

 node:net:1296
    validatePort(port);
    ^

RangeError [ERR_SOCKET_BAD_PORT]: Port should be >= 0 and < 65536. Received type number (NaN).
    at lookupAndConnect (node:net:1296:5)
    at Socket.connect (node:net:1253:5)
    at Object.connect (node:net:236:17)
    at forwardToDaemon (file:///home/user/eslint_d.js/lib/forwarder.js:41:22)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async file:///home/user/eslint_d.js/bin/eslint_d.js:67:7 {
  code: 'ERR_SOCKET_BAD_PORT'
}

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 1, 2024

config content: { token: '', port: NaN, pid: NaN, hash: undefined }

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 1, 2024

I've added check here that port not valid, now seems to not having any issues with exactly this test:

if (!config) {

but another appeared:

  1) integration tests
       v8.0.x
         stop:
     Error: Timeout of 100000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/user/eslint_d.js/test/test.integration.js)
      at listOnTimeout (node:internal/timers:581:17)
      at process.processTimers (node:internal/timers:519:7)

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 1, 2024

I've added check here that port not valid, now seems to not having any issues with exactly this test:

if (!config) {

Well, just adding check didn't help, issue is really flaky, sometime it pass, sometime no, now it has no errors and still fails, I guess it might be that daemon / eslint crashes for some reasons and that's why there's no output 🤔

@RayGuo-ergou
Copy link

Weird I am also on linux but cannot reproduce the issue.

❯ cat /etc/lsb-release
───────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: /etc/lsb-release
───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ DISTRIB_ID=Ubuntu
   2   │ DISTRIB_RELEASE=22.04
   3   │ DISTRIB_CODENAME=jammy
   4   │ DISTRIB_DESCRIPTION="Ubuntu 22.04.4 LTS"
───────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

❯ nr
✔ script to run › test:integration

> [email protected] test:integration
> mocha --bail --slow 1000 test/test.integration.js



  ..................................................

  50 passing (3s)

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 2, 2024

Are you on correct branch?

this branch contains 70 tests, in your output only 50

$ npm run test:integration -- --timeout 100000

> [email protected] test:integration
> mocha --bail --slow 1000 test/test.integration.js --timeout 100000



  ......................................................................

  70 passing (51s)

@RayGuo-ergou
Copy link

🫠Was interrupted by something and forget to change branch.

Yeah got the same error in cli

❯ nr
✔ script to run › test:integration

> [email protected] test:integration
> mocha --bail --slow 1000 test/test.integration.js



  ......................................................!

  54 passing (4s)
  1 failing

  1) integration tests
       --fix-to-stdout
         v6.0.x
           when file only contains fixable problems
             prints input if no change is needed:

      AssertionError: [assert.equals] '' expected to be equal to "console.log('Hello eslint');"
      + expected - actual

      +console.log('Hello eslint');

      at Object.fail (node_modules/@sinonjs/referee/lib/create-fail.js:5:21)
      at Object.fail (node_modules/@sinonjs/referee/lib/define-assertion/index.js:47:17)
      at assertion (node_modules/@sinonjs/referee/lib/define-assertion/index.js:65:11)
      at referee.<computed>.<computed> [as equals] (node_modules/@sinonjs/referee/lib/define-assertion/index.js:92:22)
      at Context.<anonymous> (file:///home/ray/git/eslint_d.js/test/test.integration.js:209:20)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 2, 2024

No problem, to see error you could swap assets in test this way:

assert.isNull(error);
assert.equals(stderr, '');
assert.equals(stdout, '');

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 2, 2024

Also if you run tests several time then some of runs could be successful, really weird

@RayGuo-ergou
Copy link

Same here tried run 10 times succeed once.

The version failed for me only 5 or 6, but saw 8 in CI.

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 2, 2024

As a quick-fix solution we could use retry feature from mocha, long term this flaky test should be fixed
https://mochajs.org/#retry-tests

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 2, 2024

but another appeared:

  1) integration tests
       v8.0.x
         stop:
     Error: Timeout of 100000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/user/eslint_d.js/test/test.integration.js)
      at listOnTimeout (node:internal/timers:581:17)
      at process.processTimers (node:internal/timers:519:7)

Regarding this issue, for some weird reasons it stucks on fs.unlink for very long time (maybe forever), as a solution I've replaced fs.unlink with fs.rm(file, { force:true })

@L2jLiga
Copy link
Contributor

L2jLiga commented Aug 2, 2024

        context('when file only contains fixable problems', () => {
          it('prints input if no change is needed', async () => {

replacing with

        context('when file only contains fixable problems', () => {
          it('prints input if no change is needed', async function() {
            this.retries(2);

makes this test much more robust in my environment

@mantoni mantoni force-pushed the add-fix-to-stdout-back branch 2 times, most recently from deea790 to 828fe51 Compare August 2, 2024 05:52
@mantoni
Copy link
Owner Author

mantoni commented Aug 2, 2024

@L2jLiga @RayGuo-ergou Thank you so much for helping debug this. It turns out that the startup sequence with file watches is quite aggressive regarding timings, so as far as I understand it now, it can happen that the config file appears on the file system, but the readFile call returns an empty string. Apparently the file appears but is not yet fully written. A short delay and re-reading the file seems to fix it.

This option was lost in the recent rewrite.
@RayGuo-ergou
Copy link

Happy to help and @L2jLiga did all the work 😄.

And Hi @mantoni and @DamienCassou , I really want to say thank you for make this happening!

@mantoni mantoni merged commit 72eb4f6 into master Aug 2, 2024
3 checks passed
@mantoni mantoni deleted the add-fix-to-stdout-back branch August 2, 2024 06:04
@mantoni
Copy link
Owner Author

mantoni commented Aug 2, 2024

📦 Released in v14.0.2

@mantoni mantoni mentioned this pull request Aug 2, 2024
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.

Absence of --fix-to-stdout
4 participants