-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
chore: refactor tests workflow #608
Conversation
b251e01
to
631ee56
Compare
d5ddce1
to
14ff9b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test
4f7310f
to
385cac2
Compare
|
||
on: | ||
pull_request_review: | ||
types: [submitted] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, if JR approves a PR, and Fabien approves it right after, does it mean the action will be run twice?
Or it will only be run once because there is no change between the 2 approves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the action will run again, which is not really desirable, but I don't see a way to prevent this at the moment. I still things it's better like this than run on every push, but we can discuss it.
jobs: | ||
tests: | ||
if: github.event.review.state == 'approved' | ||
timeout-minutes: 40 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would increase the timeout, probably 60 minutes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, it's not needed. Tests are taking 14min to 30 min depending on OS and quantity of jobs running. We can increase it in the future if this becomes a problem, but I think we should focus on optmizing the test run to take less time, since 14 ~ 30 min is already too much.
logger: | ||
- 'taipy/logger/**' | ||
rest: | ||
- 'taipy/rest/**' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add templates
here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add templates because the templates test is still inside the package, it needs to be moved to the root test folder. I can add but effectively will only work after the tests are moved to the correct folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR #583 from @toan-quach has already did that, however we did not merge it because of the timeout.
We need to align on this topic
32481ba
to
b78f8df
Compare
b78f8df
to
28cc58a
Compare
9a0778e
to
0c761f8
Compare
0c761f8
to
c150d23
Compare
No description provided.