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

Fix cron integration test #2275

Merged
merged 8 commits into from
Jul 25, 2019
Merged

Fix cron integration test #2275

merged 8 commits into from
Jul 25, 2019

Conversation

yux0
Copy link
Contributor

@yux0 yux0 commented Jul 25, 2019

Fix cron integration test

@yux0 yux0 requested review from yycptt and wxing1292 July 25, 2019 04:21
for i := 1; i != 4; i++ {
executionInfo := closedExecutions[i]
executionTime := executionInfo.GetExecutionTime()
s.Equal(int(0), int(executionTime/1000000000 - firstExecutionTime/1000000000 ) % 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment here explaining why %3 and /1000000000?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think we still need to check the execution time for the first execution, which should be 3s after the start time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the first execution which is the terminated one, the execution time equals to the start time

Copy link
Contributor

Choose a reason for hiding this comment

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

I think by first execution, I mean the first run, which should be continued as new.

For the last run, which is terminated, can you explain why the execution time equals to the start time? Am I missing something here?

Copy link
Contributor

Choose a reason for hiding this comment

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

When listing closed workflow, the returned execution ordered by increasing close time or decreasing close time? If the first execution is terminated, seems like it's the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latter. The first one is the termination and it will verify below

host/integration_test.go Outdated Show resolved Hide resolved
host/integration_test.go Outdated Show resolved Hide resolved
@yux0 yux0 merged commit 3197e1b into master Jul 25, 2019
@wxing1292 wxing1292 deleted the cron-int branch July 29, 2019 03:17
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