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

Tests with node16 #24

Merged
merged 10 commits into from
Nov 23, 2024
Merged

Conversation

KristjanESPERANTO
Copy link
Collaborator

The tests will run with node 16, but they will fail. I think because the test-runner in node 16 is still in experimental state.

@KID-joker
Copy link
Owner

Maybe you are right and the problem lies with devDependecies.
So, you should call cli.mjs in test.js?

@KristjanESPERANTO
Copy link
Collaborator Author

So, you should call cli.mjs in test.js?

You are right. I've changed that.

Locally I got the tests not failing with node 16 (strange is that it says it's only one test). But in the GitHub action it fails now even with other versions 🤔

@KID-joker
Copy link
Owner

I exposed the APIs, so you can test them directly.

@KristjanESPERANTO
Copy link
Collaborator Author

I exposed the APIs, so you can test them directly.

Exposing the API is a good idea! Maybe someone wants to integrate the package into their project.

For using the API I think we should add new tests. So we have tests that test the cli and others that test the API.

Locally I got the tests not failing with node 16 (strange is that it says it's only one test). But in the GitHub action it fails now even with other versions 🤔

I did some digging but didn't get much further. locally the tests work in node 16 but fail in the gitHub action. I'm not giving up yet, but I don't know if I'll find much time for it.

@KID-joker
Copy link
Owner

KID-joker commented Nov 18, 2024

I'm not giving up yet, but I don't know if I'll find much time for it.

Okay, I'll work with you. But it seems like you've already succeeded.

@KristjanESPERANTO
Copy link
Collaborator Author

Yes, it looks good now 🎉 Only one test does not work with node 16, I have deactivated this one test via if-condition. I sugggest to merge the PR and for the one test I create an issue and we take care of it afterwards.

@KID-joker
Copy link
Owner

Only one test does not work with node 16.

How did you get this result?
I tried running on node16 and it's fine.

@KID-joker
Copy link
Owner

And can you rewrite done into async/await?

test.js Show resolved Hide resolved
@KristjanESPERANTO
Copy link
Collaborator Author

KristjanESPERANTO commented Nov 20, 2024

Only one test does not work with node 16.

How did you get this result?
I tried running on node16 and it's fine.

On my local machine npm run test works also with node 16, but not when it runs in the GitHub workflow.

Now it works because I deactivated the problematic test for node 16 via if (!process.version.startsWith('v16')) {.

@KristjanESPERANTO
Copy link
Collaborator Author

And can you rewrite done into async/await?

I don't think so. In my understanding the done() function is called once all asynchronous operations within the test have finished. Maybe there is a way, but I don't know.

@KID-joker
Copy link
Owner

Only one test does not work with node 16.

How did you get this result?
I tried running on node16 and it's fine.

On my local machine npm run test works also with node 16, but not when it runs in the GitHub workflow.

Now it works because I deactivated the problematic test for node 16 via if (!process.version.startsWith('v16')) {.

It seems that the installation failed because the request was deprecated.

@KristjanESPERANTO
Copy link
Collaborator Author

It seems that the installation failed because the request was deprecated.

Wow, at the beginning I thought we wouldn't get it to work with node 16. Nice job! 👏

@KID-joker KID-joker merged commit ef96c3d into KID-joker:main Nov 23, 2024
5 checks passed
@KristjanESPERANTO KristjanESPERANTO deleted the testsWithNode16 branch November 23, 2024 08:02
@KID-joker
Copy link
Owner

It seems that the installation failed because the request was deprecated.

Wow, at the beginning I thought we wouldn't get it to work with node 16. Nice job! 👏

Everything is your help.

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.

2 participants