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

CI : Move from sleep to watch exists #748

Merged
merged 4 commits into from Dec 2, 2019
Merged

CI : Move from sleep to watch exists #748

merged 4 commits into from Dec 2, 2019

Conversation

zekth
Copy link
Contributor

@zekth zekth commented Dec 1, 2019

As mentionned here #744 (comment)

Windows CI seems a bit flaky on the IO. I've extended the sleep time for pino.destination tests.

Hope this time it will be stable on all the runs.

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.

However we should really change these tests to not have a sleep.

@zekth
Copy link
Contributor Author

zekth commented Dec 1, 2019

lgtm.

However we should really change these tests to not have a sleep.

We can move to fs.watchfile and hook on the creation of the file? This can be added into an util func with a timeout argument. https://nodejs.org/docs/latest-v6.x/api/fs.html#fs_fs_watchfile_filename_options_listener

@mcollina
Copy link
Member

mcollina commented Dec 1, 2019 via email

@zekth
Copy link
Contributor Author

zekth commented Dec 1, 2019

it would be really cool! Il 1 dic 2019, 12:01 +0100, Vincent LE GOFF notifications@github.com, ha scritto:

lgtm. > However we should really change these tests to not have a sleep. We can move to fs.watchfile and hook on the creation of the file? This can be added into an util func with a timeout argument. https://nodejs.org/docs/latest-v6.x/api/fs.html#fs_fs_watchfile_filename_options_listener — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

Ok so don't merge this one. I'll tackle it!

@zekth zekth changed the title Extend sleep for windows tests DONT MERGE : Extend sleep for windows tests Dec 1, 2019
@zekth
Copy link
Contributor Author

zekth commented Dec 1, 2019

So i've extended the timeout for the wait but it resolves when it finds the file, so on most of platforms it would be really quicker. I've moved to exists because Watch needs the file to already exists.

I'm ok with this current approach. What about you?

@zekth zekth changed the title DONT MERGE : Extend sleep for windows tests CI : Move from sleep to watch exists Dec 1, 2019
reject(new Error(`${filename} was not created with a Timeout:${timeout}`))
},
timeout
)
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind removing the timer and just add a counter in the interval?
we can also move the timeout to a constant inside this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a change. I thinks it fit your review, or may i've misunderstood 🙃

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 yes!

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants