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

Achieve 100% test coverage #370

Merged
merged 22 commits into from
Apr 28, 2024

Conversation

jean-michelet
Copy link
Contributor

Fixed #369

100%-test-coverage-fastify-autoload

I failed to cover some branches, so I have removed them. If you can find a way to fully evaluate these branches, please let me know. However, it's possible that they may be obsolete.

I've created an issues folder for issues related tests, which will need to follow this pattern: test/issues/<issue-number>/test.js. Since we're testing the autoloader, we need to create fixtures. This is why I chose to create a folder named after the issue instead of placing <issue-number>.js directly.

If you agree with this approach, I will update this PR to reflect these changes.

@jean-michelet
Copy link
Contributor Author

Um...

I'm going to investigate.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@jean-michelet jean-michelet mentioned this pull request Apr 18, 2024
@jean-michelet
Copy link
Contributor Author

Waiting for #371 to be merged.

@Uzlopak
Copy link
Contributor

Uzlopak commented Apr 19, 2024

You are targetting master, but #371 is targetting next

@jean-michelet jean-michelet changed the base branch from master to next April 19, 2024 16:04
@mcollina
Copy link
Member

Can we achieve this on master instead? I think it would be a moving target on next.

@jean-michelet
Copy link
Contributor Author

jean-michelet commented Apr 20, 2024

Can we achieve this on master instead? I think it would be a moving target on next.

Should I try to fix the failing TS test here?
Or it should be fixed in (or stay commented and fixed later) #371?

@jean-michelet jean-michelet changed the base branch from next to master April 20, 2024 17:12
@gurgunday
Copy link
Member

The test that's failing here and the one in 371 are different I think

So for this PR, I'd suggest making the tests pass and merging to master, and then focusing on next's failing test at 371

const isSWCNodeRegister = process._preload_modules && process._preload_modules.includes('@swc-node/register')
const isSWCRegister = process._preload_modules?.includes('@swc/register')
const isSWCNodeRegister = process._preload_modules?.includes('@swc-node/register')
/* istanbul ignore next - OS specific */
const isSWCNode = typeof process.env._ === 'string' && process.env._.includes('.bin/swc-node')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never evaluated on windows...

@jean-michelet
Copy link
Contributor Author

Ready For Review

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

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.

Achieve 100% test coverage
4 participants