Migration from tap to mocha

[StefanStojanovic]

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

After the make-fetch-happen update in 02480f6, GitHub Actions started failing. Migrating from tap to mocha testing framework as issues only happened in GitHub Actions (tested locally with various node/python combinations, didn't reproduce any failures). This PR is made based on investigation and work done in #2837 as the simplest and probably the cleanest solution. With these changes, Mocha replaces tap completely, thus restoring GitHub Actions stability.

[rvagg]

Still using tap for writing tests, mocha is used only for execution

Yeah, I don't think that's a good idea. The files are runnable by themselves, and will execute as soon as they are loaded into process, so all mocha is doing is acting as a "load these files" engine.

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

[StefanStojanovic]

Thanks for that information. I didn't want to rewrite tests initially as I did not know if that is the direction we want to go in. Since it is, I'll change the tests as well.

[StefanStojanovic]

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

@rvagg I've rewritten tests. I haven't used chai because assert was more than enough for what I needed. I've also made a custom mocha reporter in 436cc18 which, at first, I used for development, but it ended up being useful for the detailed test report. Potentially we can stick with it, but I'm also OK with using something prebuilt and less verbose like the tap reporter.

Regarding the PR, I'm still keeping it as a draft because I want #2650 and #2846 to land first so I can change those tests too, and not leave it to the people who opened those PRs to make needed changes. Besides waiting for those PRs, I consider this work completed.

P.S. GitHub makes it look like there are very big changes, but most of them are just indentation and changes from tap to assert. It could be easier for reviewers to use some 3rd party tools such as KDiff3.

[StefanStojanovic]

@dennisameling and @dsanders11 when you get time, please review the changes I made to the tests you added in #2650 and #2846 respectively. They were supposed to be just changed from tap to mocha, but if I made some other unintended change let me know.

[dsanders11]

@StefanStojanovic, parallel install tests refactor to mocha LGTM.

[dennisameling]

Refactor of the VS2022 tests LGTM ✅️ thank you!

[cclauss]

Will we do a similar migration in the testing of Node.js?

• https://github.com/search?q=repo%3Anodejs%2Fnode+%22+tap%22&type=code

Will we be able to drop tap2junit?

• https://github.com/nodejs/tap2junit --> https://pypi.org/project/tap2junit

[rvagg]

so for background: tap2junit is used for nice test feedback in jenkins - you can run these tests in ci.nodejs.org and get nice output

mocha can do tap output if you want, but none of this is essential, it could all be ditched I think and just get nice mocha output 🤷

[StefanStojanovic]

Will we do a similar migration in the testing of Node.js?

There is no plan to do that for Node.js tests as they are working well.

Will we be able to drop tap2junit?

These tests are run through GitHub Actions, tap2junit is not used here.

[rvagg]

These tests are run through GitHub Actions, tap2junit is not used here.

See https://ci.nodejs.org/view/All/search/?q=gyp

[cclauss]

Should these changes be landed on https://github.com/nodejs/gyp-next first?

[StefanStojanovic]

See https://ci.nodejs.org/view/All/search/?q=gyp

Are those jobs still in use? They seem outdated and have no build history. Also, I've only seen GitHub Actions in node-gyp repo PRs

Should these changes be landed on https://github.com/nodejs/gyp-next first?

I do not know what you mean by this, since these changes change node-gyp tests. From what I saw (went through it very briefly) gyp-next has a node-gyp integration workflow that runs node-gyp tests (and for that reason is in red lately), but it uses this repo, so landing this PR here should manifest there as well. If you had something else in mind please let me know.

[rvagg]

https://ci.nodejs.org/job/nodegyp-test-pull-request/

One of the benefits of this is the ability to test a broader range of Node versions and platform permutations than CI allows; which is helpful for a project that depends so much on OS-level inputs like node-gyp does. It hasn't been used much recently because the people that might have used it have slowly faded away, leaving us with CI.
I'm not suggesting it be retained as an important piece of functionality, or block the work here, just maybe not forgotten as a thing of value to maybe utilise or integrate in the CI process at some point.

[StefanStojanovic]

Since we've been discussing this PR for some time, and I see no clear objections to the proposed changes, I just wanted to ask if there's something else I should do to get it approved.

[StefanStojanovic]

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

After the make-fetch-happen update in 02480f6, GitHub Actions started failing. Migrating from tap to mocha testing framework as issues only happened in GitHub Actions (tested locally with various node/python combinations, didn't reproduce any failures). This PR is made based on investigation and work done in #2837 as the simplest and probably the cleanest solution. With these changes, Mocha replaces tap completely, thus restoring GitHub Actions stability.

[rvagg]

Still using tap for writing tests, mocha is used only for execution

Yeah, I don't think that's a good idea. The files are runnable by themselves, and will execute as soon as they are loaded into process, so all mocha is doing is acting as a "load these files" engine.

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

[StefanStojanovic]

Thanks for that information. I didn't want to rewrite tests initially as I did not know if that is the direction we want to go in. Since it is, I'll change the tests as well.

[StefanStojanovic]

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

@rvagg I've rewritten tests. I haven't used chai because assert was more than enough for what I needed. I've also made a custom mocha reporter in 436cc18 which, at first, I used for development, but it ended up being useful for the detailed test report. Potentially we can stick with it, but I'm also OK with using something prebuilt and less verbose like the tap reporter.

Regarding the PR, I'm still keeping it as a draft because I want #2650 and #2846 to land first so I can change those tests too, and not leave it to the people who opened those PRs to make needed changes. Besides waiting for those PRs, I consider this work completed.

P.S. GitHub makes it look like there are very big changes, but most of them are just indentation and changes from tap to assert. It could be easier for reviewers to use some 3rd party tools such as KDiff3.

[StefanStojanovic]

@dennisameling and @dsanders11 when you get time, please review the changes I made to the tests you added in #2650 and #2846 respectively. They were supposed to be just changed from tap to mocha, but if I made some other unintended change let me know.

[dsanders11]

@StefanStojanovic, parallel install tests refactor to mocha LGTM.

[dennisameling]

Refactor of the VS2022 tests LGTM ✅️ thank you!

[cclauss]

Will we do a similar migration in the testing of Node.js?

• https://github.com/search?q=repo%3Anodejs%2Fnode+%22+tap%22&type=code

Will we be able to drop tap2junit?

• https://github.com/nodejs/tap2junit --> https://pypi.org/project/tap2junit

[rvagg]

so for background: tap2junit is used for nice test feedback in jenkins - you can run these tests in ci.nodejs.org and get nice output

mocha can do tap output if you want, but none of this is essential, it could all be ditched I think and just get nice mocha output 🤷

[StefanStojanovic]

Will we do a similar migration in the testing of Node.js?

There is no plan to do that for Node.js tests as they are working well.

Will we be able to drop tap2junit?

These tests are run through GitHub Actions, tap2junit is not used here.

[rvagg]

These tests are run through GitHub Actions, tap2junit is not used here.

See https://ci.nodejs.org/view/All/search/?q=gyp

[cclauss]

Should these changes be landed on https://github.com/nodejs/gyp-next first?

[StefanStojanovic]

See https://ci.nodejs.org/view/All/search/?q=gyp

Are those jobs still in use? They seem outdated and have no build history. Also, I've only seen GitHub Actions in node-gyp repo PRs

Should these changes be landed on https://github.com/nodejs/gyp-next first?

I do not know what you mean by this, since these changes change node-gyp tests. From what I saw (went through it very briefly) gyp-next has a node-gyp integration workflow that runs node-gyp tests (and for that reason is in red lately), but it uses this repo, so landing this PR here should manifest there as well. If you had something else in mind please let me know.

[rvagg]

https://ci.nodejs.org/job/nodegyp-test-pull-request/

One of the benefits of this is the ability to test a broader range of Node versions and platform permutations than CI allows; which is helpful for a project that depends so much on OS-level inputs like node-gyp does. It hasn't been used much recently because the people that might have used it have slowly faded away, leaving us with CI.
I'm not suggesting it be retained as an important piece of functionality, or block the work here, just maybe not forgotten as a thing of value to maybe utilise or integrate in the CI process at some point.

[StefanStojanovic]

Since we've been discussing this PR for some time, and I see no clear objections to the proposed changes, I just wanted to ask if there's something else I should do to get it approved.

[StefanStojanovic]

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

After the make-fetch-happen update in 02480f6, GitHub Actions started failing. Migrating from tap to mocha testing framework as issues only happened in GitHub Actions (tested locally with various node/python combinations, didn't reproduce any failures). This PR is made based on investigation and work done in #2837 as the simplest and probably the cleanest solution. With these changes, Mocha replaces tap completely, thus restoring GitHub Actions stability.

[rvagg]

Still using tap for writing tests, mocha is used only for execution

Yeah, I don't think that's a good idea. The files are runnable by themselves, and will execute as soon as they are loaded into process, so all mocha is doing is acting as a "load these files" engine.

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

[StefanStojanovic]

Thanks for that information. I didn't want to rewrite tests initially as I did not know if that is the direction we want to go in. Since it is, I'll change the tests as well.

[StefanStojanovic]

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

@rvagg I've rewritten tests. I haven't used chai because assert was more than enough for what I needed. I've also made a custom mocha reporter in 436cc18 which, at first, I used for development, but it ended up being useful for the detailed test report. Potentially we can stick with it, but I'm also OK with using something prebuilt and less verbose like the tap reporter.

Regarding the PR, I'm still keeping it as a draft because I want #2650 and #2846 to land first so I can change those tests too, and not leave it to the people who opened those PRs to make needed changes. Besides waiting for those PRs, I consider this work completed.

P.S. GitHub makes it look like there are very big changes, but most of them are just indentation and changes from tap to assert. It could be easier for reviewers to use some 3rd party tools such as KDiff3.

[StefanStojanovic]

@dennisameling and @dsanders11 when you get time, please review the changes I made to the tests you added in #2650 and #2846 respectively. They were supposed to be just changed from tap to mocha, but if I made some other unintended change let me know.

[dsanders11]

@StefanStojanovic, parallel install tests refactor to mocha LGTM.

[dennisameling]

Refactor of the VS2022 tests LGTM ✅️ thank you!

[cclauss]

Will we do a similar migration in the testing of Node.js?

• https://github.com/search?q=repo%3Anodejs%2Fnode+%22+tap%22&type=code

Will we be able to drop tap2junit?

• https://github.com/nodejs/tap2junit --> https://pypi.org/project/tap2junit

[rvagg]

so for background: tap2junit is used for nice test feedback in jenkins - you can run these tests in ci.nodejs.org and get nice output

mocha can do tap output if you want, but none of this is essential, it could all be ditched I think and just get nice mocha output 🤷

[StefanStojanovic]

Will we do a similar migration in the testing of Node.js?

There is no plan to do that for Node.js tests as they are working well.

Will we be able to drop tap2junit?

These tests are run through GitHub Actions, tap2junit is not used here.

[rvagg]

These tests are run through GitHub Actions, tap2junit is not used here.

See https://ci.nodejs.org/view/All/search/?q=gyp

[cclauss]

Should these changes be landed on https://github.com/nodejs/gyp-next first?

[StefanStojanovic]

See https://ci.nodejs.org/view/All/search/?q=gyp

Are those jobs still in use? They seem outdated and have no build history. Also, I've only seen GitHub Actions in node-gyp repo PRs

Should these changes be landed on https://github.com/nodejs/gyp-next first?

I do not know what you mean by this, since these changes change node-gyp tests. From what I saw (went through it very briefly) gyp-next has a node-gyp integration workflow that runs node-gyp tests (and for that reason is in red lately), but it uses this repo, so landing this PR here should manifest there as well. If you had something else in mind please let me know.

[rvagg]

https://ci.nodejs.org/job/nodegyp-test-pull-request/

One of the benefits of this is the ability to test a broader range of Node versions and platform permutations than CI allows; which is helpful for a project that depends so much on OS-level inputs like node-gyp does. It hasn't been used much recently because the people that might have used it have slowly faded away, leaving us with CI.
I'm not suggesting it be retained as an important piece of functionality, or block the work here, just maybe not forgotten as a thing of value to maybe utilise or integrate in the CI process at some point.

[StefanStojanovic]

Since we've been discussing this PR for some time, and I see no clear objections to the proposed changes, I just wanted to ask if there's something else I should do to get it approved.

[StefanStojanovic]

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

After the make-fetch-happen update in 02480f6, GitHub Actions started failing. Migrating from tap to mocha testing framework as issues only happened in GitHub Actions (tested locally with various node/python combinations, didn't reproduce any failures). This PR is made based on investigation and work done in #2837 as the simplest and probably the cleanest solution. With these changes, Mocha replaces tap completely, thus restoring GitHub Actions stability.

[rvagg]

Still using tap for writing tests, mocha is used only for execution

Yeah, I don't think that's a good idea. The files are runnable by themselves, and will execute as soon as they are loaded into process, so all mocha is doing is acting as a "load these files" engine.

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

[StefanStojanovic]

Thanks for that information. I didn't want to rewrite tests initially as I did not know if that is the direction we want to go in. Since it is, I'll change the tests as well.

[StefanStojanovic]

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

@rvagg I've rewritten tests. I haven't used chai because assert was more than enough for what I needed. I've also made a custom mocha reporter in 436cc18 which, at first, I used for development, but it ended up being useful for the detailed test report. Potentially we can stick with it, but I'm also OK with using something prebuilt and less verbose like the tap reporter.

Regarding the PR, I'm still keeping it as a draft because I want #2650 and #2846 to land first so I can change those tests too, and not leave it to the people who opened those PRs to make needed changes. Besides waiting for those PRs, I consider this work completed.

P.S. GitHub makes it look like there are very big changes, but most of them are just indentation and changes from tap to assert. It could be easier for reviewers to use some 3rd party tools such as KDiff3.

[StefanStojanovic]

@dennisameling and @dsanders11 when you get time, please review the changes I made to the tests you added in #2650 and #2846 respectively. They were supposed to be just changed from tap to mocha, but if I made some other unintended change let me know.

[dsanders11]

@StefanStojanovic, parallel install tests refactor to mocha LGTM.

[dennisameling]

Refactor of the VS2022 tests LGTM ✅️ thank you!

[cclauss]

Will we do a similar migration in the testing of Node.js?

• https://github.com/search?q=repo%3Anodejs%2Fnode+%22+tap%22&type=code

Will we be able to drop tap2junit?

• https://github.com/nodejs/tap2junit --> https://pypi.org/project/tap2junit

[rvagg]

so for background: tap2junit is used for nice test feedback in jenkins - you can run these tests in ci.nodejs.org and get nice output

mocha can do tap output if you want, but none of this is essential, it could all be ditched I think and just get nice mocha output 🤷

[StefanStojanovic]

Will we do a similar migration in the testing of Node.js?

There is no plan to do that for Node.js tests as they are working well.

Will we be able to drop tap2junit?

These tests are run through GitHub Actions, tap2junit is not used here.

[rvagg]

These tests are run through GitHub Actions, tap2junit is not used here.

See https://ci.nodejs.org/view/All/search/?q=gyp

[cclauss]

Should these changes be landed on https://github.com/nodejs/gyp-next first?

[StefanStojanovic]

See https://ci.nodejs.org/view/All/search/?q=gyp

Are those jobs still in use? They seem outdated and have no build history. Also, I've only seen GitHub Actions in node-gyp repo PRs

Should these changes be landed on https://github.com/nodejs/gyp-next first?

I do not know what you mean by this, since these changes change node-gyp tests. From what I saw (went through it very briefly) gyp-next has a node-gyp integration workflow that runs node-gyp tests (and for that reason is in red lately), but it uses this repo, so landing this PR here should manifest there as well. If you had something else in mind please let me know.

[rvagg]

https://ci.nodejs.org/job/nodegyp-test-pull-request/

One of the benefits of this is the ability to test a broader range of Node versions and platform permutations than CI allows; which is helpful for a project that depends so much on OS-level inputs like node-gyp does. It hasn't been used much recently because the people that might have used it have slowly faded away, leaving us with CI.
I'm not suggesting it be retained as an important piece of functionality, or block the work here, just maybe not forgotten as a thing of value to maybe utilise or integrate in the CI process at some point.

[StefanStojanovic]

Since we've been discussing this PR for some time, and I see no clear objections to the proposed changes, I just wanted to ask if there's something else I should do to get it approved.

[StefanStojanovic]

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

After the make-fetch-happen update in 02480f6, GitHub Actions started failing. Migrating from tap to mocha testing framework as issues only happened in GitHub Actions (tested locally with various node/python combinations, didn't reproduce any failures). This PR is made based on investigation and work done in #2837 as the simplest and probably the cleanest solution. With these changes, Mocha replaces tap completely, thus restoring GitHub Actions stability.

[rvagg]

Still using tap for writing tests, mocha is used only for execution

Yeah, I don't think that's a good idea. The files are runnable by themselves, and will execute as soon as they are loaded into process, so all mocha is doing is acting as a "load these files" engine.

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

[StefanStojanovic]

Thanks for that information. I didn't want to rewrite tests initially as I did not know if that is the direction we want to go in. Since it is, I'll change the tests as well.

[StefanStojanovic]

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

@rvagg I've rewritten tests. I haven't used chai because assert was more than enough for what I needed. I've also made a custom mocha reporter in 436cc18 which, at first, I used for development, but it ended up being useful for the detailed test report. Potentially we can stick with it, but I'm also OK with using something prebuilt and less verbose like the tap reporter.

Regarding the PR, I'm still keeping it as a draft because I want #2650 and #2846 to land first so I can change those tests too, and not leave it to the people who opened those PRs to make needed changes. Besides waiting for those PRs, I consider this work completed.

P.S. GitHub makes it look like there are very big changes, but most of them are just indentation and changes from tap to assert. It could be easier for reviewers to use some 3rd party tools such as KDiff3.

[StefanStojanovic]

@dennisameling and @dsanders11 when you get time, please review the changes I made to the tests you added in #2650 and #2846 respectively. They were supposed to be just changed from tap to mocha, but if I made some other unintended change let me know.

[dsanders11]

@StefanStojanovic, parallel install tests refactor to mocha LGTM.

[dennisameling]

Refactor of the VS2022 tests LGTM ✅️ thank you!

[cclauss]

Will we do a similar migration in the testing of Node.js?

• https://github.com/search?q=repo%3Anodejs%2Fnode+%22+tap%22&type=code

Will we be able to drop tap2junit?

• https://github.com/nodejs/tap2junit --> https://pypi.org/project/tap2junit

[rvagg]

so for background: tap2junit is used for nice test feedback in jenkins - you can run these tests in ci.nodejs.org and get nice output

mocha can do tap output if you want, but none of this is essential, it could all be ditched I think and just get nice mocha output 🤷

[StefanStojanovic]

Will we do a similar migration in the testing of Node.js?

There is no plan to do that for Node.js tests as they are working well.

Will we be able to drop tap2junit?

These tests are run through GitHub Actions, tap2junit is not used here.

[rvagg]

These tests are run through GitHub Actions, tap2junit is not used here.

See https://ci.nodejs.org/view/All/search/?q=gyp

[cclauss]

Should these changes be landed on https://github.com/nodejs/gyp-next first?

[StefanStojanovic]

See https://ci.nodejs.org/view/All/search/?q=gyp

Are those jobs still in use? They seem outdated and have no build history. Also, I've only seen GitHub Actions in node-gyp repo PRs

Should these changes be landed on https://github.com/nodejs/gyp-next first?

I do not know what you mean by this, since these changes change node-gyp tests. From what I saw (went through it very briefly) gyp-next has a node-gyp integration workflow that runs node-gyp tests (and for that reason is in red lately), but it uses this repo, so landing this PR here should manifest there as well. If you had something else in mind please let me know.

[rvagg]

https://ci.nodejs.org/job/nodegyp-test-pull-request/

One of the benefits of this is the ability to test a broader range of Node versions and platform permutations than CI allows; which is helpful for a project that depends so much on OS-level inputs like node-gyp does. It hasn't been used much recently because the people that might have used it have slowly faded away, leaving us with CI.
I'm not suggesting it be retained as an important piece of functionality, or block the work here, just maybe not forgotten as a thing of value to maybe utilise or integrate in the CI process at some point.

[StefanStojanovic]

Since we've been discussing this PR for some time, and I see no clear objections to the proposed changes, I just wanted to ask if there's something else I should do to get it approved.

[StefanStojanovic]

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

After the make-fetch-happen update in 02480f6, GitHub Actions started failing. Migrating from tap to mocha testing framework as issues only happened in GitHub Actions (tested locally with various node/python combinations, didn't reproduce any failures). This PR is made based on investigation and work done in #2837 as the simplest and probably the cleanest solution. With these changes, Mocha replaces tap completely, thus restoring GitHub Actions stability.

[rvagg]

Still using tap for writing tests, mocha is used only for execution

Yeah, I don't think that's a good idea. The files are runnable by themselves, and will execute as soon as they are loaded into process, so all mocha is doing is acting as a "load these files" engine.

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

[StefanStojanovic]

Thanks for that information. I didn't want to rewrite tests initially as I did not know if that is the direction we want to go in. Since it is, I'll change the tests as well.

[StefanStojanovic]

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

@rvagg I've rewritten tests. I haven't used chai because assert was more than enough for what I needed. I've also made a custom mocha reporter in 436cc18 which, at first, I used for development, but it ended up being useful for the detailed test report. Potentially we can stick with it, but I'm also OK with using something prebuilt and less verbose like the tap reporter.

Regarding the PR, I'm still keeping it as a draft because I want #2650 and #2846 to land first so I can change those tests too, and not leave it to the people who opened those PRs to make needed changes. Besides waiting for those PRs, I consider this work completed.

P.S. GitHub makes it look like there are very big changes, but most of them are just indentation and changes from tap to assert. It could be easier for reviewers to use some 3rd party tools such as KDiff3.

[StefanStojanovic]

@dennisameling and @dsanders11 when you get time, please review the changes I made to the tests you added in #2650 and #2846 respectively. They were supposed to be just changed from tap to mocha, but if I made some other unintended change let me know.

[dsanders11]

@StefanStojanovic, parallel install tests refactor to mocha LGTM.

[dennisameling]

Refactor of the VS2022 tests LGTM ✅️ thank you!

[cclauss]

Will we do a similar migration in the testing of Node.js?

• https://github.com/search?q=repo%3Anodejs%2Fnode+%22+tap%22&type=code

Will we be able to drop tap2junit?

• https://github.com/nodejs/tap2junit --> https://pypi.org/project/tap2junit

[rvagg]

so for background: tap2junit is used for nice test feedback in jenkins - you can run these tests in ci.nodejs.org and get nice output

mocha can do tap output if you want, but none of this is essential, it could all be ditched I think and just get nice mocha output 🤷

[StefanStojanovic]

Will we do a similar migration in the testing of Node.js?

There is no plan to do that for Node.js tests as they are working well.

Will we be able to drop tap2junit?

These tests are run through GitHub Actions, tap2junit is not used here.

[rvagg]

These tests are run through GitHub Actions, tap2junit is not used here.

See https://ci.nodejs.org/view/All/search/?q=gyp

[cclauss]

Should these changes be landed on https://github.com/nodejs/gyp-next first?

[StefanStojanovic]

See https://ci.nodejs.org/view/All/search/?q=gyp

Are those jobs still in use? They seem outdated and have no build history. Also, I've only seen GitHub Actions in node-gyp repo PRs

Should these changes be landed on https://github.com/nodejs/gyp-next first?

I do not know what you mean by this, since these changes change node-gyp tests. From what I saw (went through it very briefly) gyp-next has a node-gyp integration workflow that runs node-gyp tests (and for that reason is in red lately), but it uses this repo, so landing this PR here should manifest there as well. If you had something else in mind please let me know.

[rvagg]

https://ci.nodejs.org/job/nodegyp-test-pull-request/

One of the benefits of this is the ability to test a broader range of Node versions and platform permutations than CI allows; which is helpful for a project that depends so much on OS-level inputs like node-gyp does. It hasn't been used much recently because the people that might have used it have slowly faded away, leaving us with CI.
I'm not suggesting it be retained as an important piece of functionality, or block the work here, just maybe not forgotten as a thing of value to maybe utilise or integrate in the CI process at some point.

[StefanStojanovic]

Since we've been discussing this PR for some time, and I see no clear objections to the proposed changes, I just wanted to ask if there's something else I should do to get it approved.

[StefanStojanovic]

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

After the make-fetch-happen update in 02480f6, GitHub Actions started failing. Migrating from tap to mocha testing framework as issues only happened in GitHub Actions (tested locally with various node/python combinations, didn't reproduce any failures). This PR is made based on investigation and work done in #2837 as the simplest and probably the cleanest solution. With these changes, Mocha replaces tap completely, thus restoring GitHub Actions stability.

[rvagg]

Still using tap for writing tests, mocha is used only for execution

Yeah, I don't think that's a good idea. The files are runnable by themselves, and will execute as soon as they are loaded into process, so all mocha is doing is acting as a "load these files" engine.

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

[StefanStojanovic]

Thanks for that information. I didn't want to rewrite tests initially as I did not know if that is the direction we want to go in. Since it is, I'll change the tests as well.

[StefanStojanovic]

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

@rvagg I've rewritten tests. I haven't used chai because assert was more than enough for what I needed. I've also made a custom mocha reporter in 436cc18 which, at first, I used for development, but it ended up being useful for the detailed test report. Potentially we can stick with it, but I'm also OK with using something prebuilt and less verbose like the tap reporter.

Regarding the PR, I'm still keeping it as a draft because I want #2650 and #2846 to land first so I can change those tests too, and not leave it to the people who opened those PRs to make needed changes. Besides waiting for those PRs, I consider this work completed.

P.S. GitHub makes it look like there are very big changes, but most of them are just indentation and changes from tap to assert. It could be easier for reviewers to use some 3rd party tools such as KDiff3.

[StefanStojanovic]

@dennisameling and @dsanders11 when you get time, please review the changes I made to the tests you added in #2650 and #2846 respectively. They were supposed to be just changed from tap to mocha, but if I made some other unintended change let me know.

[dsanders11]

@StefanStojanovic, parallel install tests refactor to mocha LGTM.

[dennisameling]

Refactor of the VS2022 tests LGTM ✅️ thank you!

[cclauss]

Will we do a similar migration in the testing of Node.js?

• https://github.com/search?q=repo%3Anodejs%2Fnode+%22+tap%22&type=code

Will we be able to drop tap2junit?

• https://github.com/nodejs/tap2junit --> https://pypi.org/project/tap2junit

[rvagg]

so for background: tap2junit is used for nice test feedback in jenkins - you can run these tests in ci.nodejs.org and get nice output

mocha can do tap output if you want, but none of this is essential, it could all be ditched I think and just get nice mocha output 🤷

[StefanStojanovic]

Will we do a similar migration in the testing of Node.js?

There is no plan to do that for Node.js tests as they are working well.

Will we be able to drop tap2junit?

These tests are run through GitHub Actions, tap2junit is not used here.

[rvagg]

These tests are run through GitHub Actions, tap2junit is not used here.

See https://ci.nodejs.org/view/All/search/?q=gyp

[cclauss]

Should these changes be landed on https://github.com/nodejs/gyp-next first?

[StefanStojanovic]

See https://ci.nodejs.org/view/All/search/?q=gyp

Are those jobs still in use? They seem outdated and have no build history. Also, I've only seen GitHub Actions in node-gyp repo PRs

Should these changes be landed on https://github.com/nodejs/gyp-next first?

I do not know what you mean by this, since these changes change node-gyp tests. From what I saw (went through it very briefly) gyp-next has a node-gyp integration workflow that runs node-gyp tests (and for that reason is in red lately), but it uses this repo, so landing this PR here should manifest there as well. If you had something else in mind please let me know.

[rvagg]

https://ci.nodejs.org/job/nodegyp-test-pull-request/

One of the benefits of this is the ability to test a broader range of Node versions and platform permutations than CI allows; which is helpful for a project that depends so much on OS-level inputs like node-gyp does. It hasn't been used much recently because the people that might have used it have slowly faded away, leaving us with CI.
I'm not suggesting it be retained as an important piece of functionality, or block the work here, just maybe not forgotten as a thing of value to maybe utilise or integrate in the CI process at some point.

[StefanStojanovic]

Since we've been discussing this PR for some time, and I see no clear objections to the proposed changes, I just wanted to ask if there's something else I should do to get it approved.

[StefanStojanovic]

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

After the make-fetch-happen update in 02480f6, GitHub Actions started failing. Migrating from tap to mocha testing framework as issues only happened in GitHub Actions (tested locally with various node/python combinations, didn't reproduce any failures). This PR is made based on investigation and work done in #2837 as the simplest and probably the cleanest solution. With these changes, Mocha replaces tap completely, thus restoring GitHub Actions stability.

[rvagg]

Still using tap for writing tests, mocha is used only for execution

Yeah, I don't think that's a good idea. The files are runnable by themselves, and will execute as soon as they are loaded into process, so all mocha is doing is acting as a "load these files" engine.

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

[StefanStojanovic]

Thanks for that information. I didn't want to rewrite tests initially as I did not know if that is the direction we want to go in. Since it is, I'll change the tests as well.

[StefanStojanovic]

Actually rewriting the tests to Mocha+Chai (or similar) might be a decent path forward though, I know we've got a bit stuck with tap a few times.

@rvagg I've rewritten tests. I haven't used chai because assert was more than enough for what I needed. I've also made a custom mocha reporter in 436cc18 which, at first, I used for development, but it ended up being useful for the detailed test report. Potentially we can stick with it, but I'm also OK with using something prebuilt and less verbose like the tap reporter.

Regarding the PR, I'm still keeping it as a draft because I want #2650 and #2846 to land first so I can change those tests too, and not leave it to the people who opened those PRs to make needed changes. Besides waiting for those PRs, I consider this work completed.

P.S. GitHub makes it look like there are very big changes, but most of them are just indentation and changes from tap to assert. It could be easier for reviewers to use some 3rd party tools such as KDiff3.

[StefanStojanovic]

@dennisameling and @dsanders11 when you get time, please review the changes I made to the tests you added in #2650 and #2846 respectively. They were supposed to be just changed from tap to mocha, but if I made some other unintended change let me know.

[dsanders11]

@StefanStojanovic, parallel install tests refactor to mocha LGTM.

[dennisameling]

Refactor of the VS2022 tests LGTM ✅️ thank you!

[cclauss]

Will we do a similar migration in the testing of Node.js?

• https://github.com/search?q=repo%3Anodejs%2Fnode+%22+tap%22&type=code

Will we be able to drop tap2junit?

• https://github.com/nodejs/tap2junit --> https://pypi.org/project/tap2junit

[rvagg]

so for background: tap2junit is used for nice test feedback in jenkins - you can run these tests in ci.nodejs.org and get nice output

mocha can do tap output if you want, but none of this is essential, it could all be ditched I think and just get nice mocha output 🤷

[StefanStojanovic]

Will we do a similar migration in the testing of Node.js?

There is no plan to do that for Node.js tests as they are working well.

Will we be able to drop tap2junit?

These tests are run through GitHub Actions, tap2junit is not used here.

[rvagg]

These tests are run through GitHub Actions, tap2junit is not used here.

See https://ci.nodejs.org/view/All/search/?q=gyp

[cclauss]

Should these changes be landed on https://github.com/nodejs/gyp-next first?

[StefanStojanovic]

See https://ci.nodejs.org/view/All/search/?q=gyp

Are those jobs still in use? They seem outdated and have no build history. Also, I've only seen GitHub Actions in node-gyp repo PRs

Should these changes be landed on https://github.com/nodejs/gyp-next first?

I do not know what you mean by this, since these changes change node-gyp tests. From what I saw (went through it very briefly) gyp-next has a node-gyp integration workflow that runs node-gyp tests (and for that reason is in red lately), but it uses this repo, so landing this PR here should manifest there as well. If you had something else in mind please let me know.

[rvagg]

https://ci.nodejs.org/job/nodegyp-test-pull-request/

One of the benefits of this is the ability to test a broader range of Node versions and platform permutations than CI allows; which is helpful for a project that depends so much on OS-level inputs like node-gyp does. It hasn't been used much recently because the people that might have used it have slowly faded away, leaving us with CI.
I'm not suggesting it be retained as an important piece of functionality, or block the work here, just maybe not forgotten as a thing of value to maybe utilise or integrate in the CI process at some point.

[StefanStojanovic]

Since we've been discussing this PR for some time, and I see no clear objections to the proposed changes, I just wanted to ask if there's something else I should do to get it approved.