5.x tests on windows

[n4bb12]

1. The PR adds windows to the CI matrix
2. The PR fixes some tests that were failing on windows by using a pattern instead of hardcoded strings.
3. Some tiny snapshots got converted to inline snapshots (imho makes it easier to understand tests)
4. The lint job didn't run on GitHub's windows image due to line ending issues. I fixed this by adding a .gitattributes with lf as the default line ending for text
5. The PR sets one test on skip because it doesn't exit normally on windows, apparently due to a node.js streaming issue or something
6. Fixes website/gatsby-node.js on windows and simplifies it

[DanielRuf]

Awesome @n4bb12 👍

I guess you are still working on this and will add the missing strategies with the matrix setup.
Let us know if there is anything that we can help you with.

[n4bb12]

@DanielRuf the website job is failing for me

But it seems to already be misconfigured on the base branch since there is no yarn lockfile in the repository. I don't think this is related to any of the PR changes.

[n4bb12]

I looked at the 5.x branch and the website jobs don't seem to run there. What am I missing?

[n4bb12]

It seems there's an issue with pino not properly exiting on windows.
Same issue when running tests locally on windows.

packages/verdaccio test: Test Suites: 1 passed, 1 total
packages/verdaccio test: Tests:       3 skipped, 152 passed, 155 total
packages/verdaccio test: Snapshots:   0 total
packages/verdaccio test: Time:        18.652 s
packages/verdaccio test: Ran all test suites matching /.\\test\\functional\\index*/i.
packages/verdaccio test:   ●  process.exit called with "1"
packages/verdaccio test:       at process.on._pino.default.final (../../logger/build/logger.js:162:13)
packages/verdaccio test:       at process.args (../../../node_modules/.pnpm/pino@5.17.0/node_modules/pino/lib/tools.js:354:12)
packages/verdaccio test: Failed
undefined
D:\a\verdaccio\verdaccio\packages\verdaccio:
 ERROR  verdaccio@5.0.0-alpha.0 test: `cross-env NODE_ENV=test jest --config ./test/jest.config.functional.js --testPathPattern ./test/functional/index* --passWithNoTests`
Exit status 1
 ERROR  Test failed. See above for more details.
##[error]Process completed with exit code 1.

[n4bb12]

packages/verdaccio test:  warn --- pino.final with prettyPrint does not support flushing
packages/verdaccio test:  fatal--- uncaughtException

This happens somewhere in the middle of the test summary.

[n4bb12]

This might actually be happening due to verdaccio's use of pino.

[DanielRuf]

But it seems to already be misconfigured on the base branch since there is no yarn lockfile in the repository. I don't think this is related to any of the PR changes.

Correct. This job is triggered by specific conditions.

verdaccio/.github/workflows/ci-website.yml

Lines 4 to 10 in 33eb429

	push:
	branches:
	- master
	pull_request:
	paths:
	- .github/workflows/ci-website.yml
	- 'website/**'

  push:
    branches:
      - master
  pull_request:
    paths:
      - .github/workflows/ci-website.yml
      - 'website/**'

I'm not sure if @juanpicado is already aware of this issue. As far as I can see we moved to pnpmjs. So the yarn command is not used anymore. You can probably change this to the correct pnpm command.

[n4bb12]

Thanks.

You can probably change this to the correct pnpm command.

I would prefer not to make yarn->pnpm changes in this PR.
Right now I'm only interested in getting tests to run on windows since it's impossible to contribute anything without being able to run tests.
I think the PR can be merged with the website jobs failing. They are failing here but they didn't break here. They only run because windows was added to the matrix. Nothing else changed.

[DanielRuf]

It seems there's an issue with pino not properly exiting on windows.

Good catch. Not sure if this is an issue caused by pino. We use pino 5.17.0.
https://github.com/verdaccio/verdaccio/blob/5.x/packages/logger/package.json#L27

The latest is pino 6.5.1:
https://github.com/pinojs/pino/releases

Maybe you can check the test which causes this.
Is the error always at this place?

packages/verdaccio test:       handling packages
packages/verdaccio test:         √ creating new package
packages/verdaccio test:         √ downloading non-existent tarball (3 ms)
packages/verdaccio test:  warn --- pino.final with prettyPrint does not support flushing
packages/verdaccio test:  fatal--- uncaughtException
packages/verdaccio test:         √ uploading incomplete tarball (22 ms)

I'm not yet sure what causes this on Windows but not on other platforms (and why).

[n4bb12]

Thanks for helping out.

Yes, it looks like it's always printed after the downloading non-existent tarball test.

If running just the packages/verdaccio tests where downloading non-existent tarball is included, it fails and the warning/fatal gets printed at the end of the output.
But if running just the packages/auth tests for example, it exits normally and I also don't see the pino warning.

Despite the warning, it might not be related to pino, that's just what seemed to be a recurring pattern.

[n4bb12]

This single test passes but exits 1 on windows: should not store tarballs / chunked

Test Suites: 1 passed, 1 total
Tests:       153 skipped, 1 passed, 154 total
Snapshots:   0 total
Time:        12.623 s, estimated 31 s
Ran all test suites matching /.\\test\\functional\\index*/i.
Teardown Verdaccio Functional
Teardown Test Environment.
 warn --- pino.final with prettyPrint does not support flushing
 fatal--- uncaughtException
  ●  process.exit called with "1"



      at ../../logger/build/logger.js:162:13
      at process.<anonymous> (../../../node_modules/.pnpm/pino@6.5.1/node_modules/pino/lib/tools.js:393:12)
 ERROR  Test failed. See above for more details.

[n4bb12]

After commenting this part in verdaccio's logger and rebuilding it

  // process.on(
  //   'unhandledRejection',
  //   pino.final(logger, (err, finalLogger) => {
  //     finalLogger.fatal(err, 'uncaughtException');
  //     process.exit(1);
  //   })
  // );

(btw, is it intentional that 'unhandledRejection' gets logged as 'uncaughtException' ?)

I get the following error that previously got swollowed by pino:

[n4bb12]

Seems to be related nodejs/node#27916 (comment)

[n4bb12]

The "should not store tarballs" test is not typed and some of the code is years old. I don't know how to fix it and I don't really want to spend much more time on this. 😒

[juanpicado]

After commenting this part in verdaccio's logger and rebuilding it

  // process.on(
  //   'unhandledRejection',
  //   pino.final(logger, (err, finalLogger) => {
  //     finalLogger.fatal(err, 'uncaughtException');
  //     process.exit(1);
  //   })
  // );

(btw, is it intentional that 'unhandledRejection' gets logged as 'uncaughtException' ?)

I get the following error that previously got swollowed by pino:

Yes, it is. Refers to pinojs/pino#528

When I implemented Pino.js to replace the deprecated bunyan-node #1334 and #1760 I did not realize they don't recommend use prettifier for production environments and pino.final throw a warning pinojs/pino-pretty#37 so I uncommented out until I fixed. It may deserve a comment for sure, my bad.

I'm not sure how to fix it, I'd need ideas, but, what I have on mind by far is start to use env parameters for prod and dev to verdaccio, and do not allow prettier in prod environments.

[juanpicado]

the website job is failing for me

Let me fix that in a separated PR. I forgot update that last week.

[juanpicado]

The "should not store tarballs" test is not typed and some of the code is years old. I don't know how to fix it and I don't really want to spend much more time on this. 😒

That unit test should be refactored and using new tooling we have already in place like nock. I've been removing those old test lately and make then more easy to maintain. I'd vote for skip it, let's ad a comment and a `test.todo('xxx') so we don't forget.

[n4bb12]

Yes, it is. Refers to pinojs/pino#528

Sorry, I wasn't referring to the pino warning, I was referring to 'unhandledRejection' gets logged as 'uncaughtException'.

[n4bb12]

[DanielRuf]

Did you already accept the invitation? =)

[DanielRuf]

LGTM, great work =) 👍

[n4bb12]

Did you already accept the invitation? =)

I accepted the invitation and I can delete branches but not merge prs.

[juanpicado]

Actually you have rights to write. No need use a fork

[n4bb12]

I'm not re-creating the PR just to merge it myself. If you care about the PR, just one of you guys click on merge please.

[DanielRuf]

I'll check why the CI is stuck. Maybe this is the problem why you can not merge it. Because this can be only merged with administrator rights when the required checks are not fulfilled.

[DanielRuf]

Hm weird, is this resolved / fixed in the target branch?

[DanielRuf]

Isn't the matrix setup still missing here?

[DanielRuf]

Nvm, seems the branch is updated now.

[n4bb12]

yes, I think so
I'll update the configs

[DanielRuf]

Is this file needed for the tests? =)

[n4bb12]

#1919 (comment)

[DanielRuf]

Ah, thanks. I forgot about this.

Is the merge button activated for you now?

[n4bb12]

[DanielRuf]

Ok, then I'll merge this. Thanks for the feedback / screenshot.

[juanpicado]

@DanielRuf @n4bb12 sorry the required checks for this branch were a bit mess up, now it is fine.

[juanpicado]

Hm weird, is this resolved / fixed in the target branch?

Code scanning is limited to master by now, since this branch still not published anywhere yet, can be re enabled anytime. Otherwise was blocking to merge.

[n4bb12]

I'll fix the runs-on config mentioned by @DanielRuf

[DanielRuf]

I'll fix the runs-on config mentioned by @DanielRuf

Currently not needed, can be also done in a separate PR =)

Thank you very much for your contribution to verdaccio 👍 Welcome as collaborator 🎉