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.

Sign up for GitHub

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

Jump to bottom

Run the timeout test 10 times 📈 #771

Closed
wants to merge 7 commits into from
Closed

Run the timeout test 10 times 📈 #771

wants to merge 7 commits into from

Conversation

bobcatfish
Copy link
Collaborator

@bobcatfish bobcatfish commented Apr 17, 2019 •
edited
Loading

If running it once doesn't always reproduce the issue, maybe running it 10 times will.

(Basicaly trying to narrow in on a repro case!)

Investigating #731

@bobcatfish
Re-enable flakey test so I can see it in action 🤓
4a43a29
@bobcatfish
Can I reproduce it with just the timeout test alone? 🎵
b2968d7 
One is the loneliest number
One is the loneliest number
One is the loneliest number that you'll ever do
One is the loneliest
One is the loneliest
One is the loneliest number that you'll ever do
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 17, 2019
@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 17, 2019
@bobcatfish
Copy link
Collaborator Author 

test/timeout_test.go:222:3: expected '}', found 'EOF'

🤦‍♀️

@bobcatfish bobcatfish force-pushed the much_timeout branch 2 times, most recently from 831e470 to 315d198 Compare April 17, 2019 23:02
@bobcatfish
Copy link
Collaborator Author

image

@bobcatfish
Run the timeout test 10 times 📈
6be9c4f 
If running it once doesn't always reproduce the issue, maybe running it 10 times will.

(Basicaly trying to narrow in on a repro case!)
@bobcatfish
Copy link
Collaborator Author 

I0417 23:24:30.743]     --- FAIL: TestTaskRunTimeout/Timeout-1 (600.37s)
I0417 23:24:30.743]         init_test.go:100: Create namespace arendelle-zj2kl to deploy to
I0417 23:24:30.743]         init_test.go:112: Verify SA "default" is created in namespace "arendelle-zj2kl"
I0417 23:24:30.744]         timeout_test.go:191: Creating Task and TaskRun in namespace arendelle-zj2kl
I0417 23:24:30.744]         timeout_test.go:203: Waiting for TaskRun run-giraffe in namespace arendelle-zj2kl to complete
I0417 23:24:30.744]         timeout_test.go:219: Error waiting for TaskRun run-giraffe to finish: timed out waiting for the condition

YUUUUSSSS

@bobcatfish
Copy link
Collaborator Author

agaaaaain

/test pull-tekton-pipeline-integration-tests

@bobcatfish
Copy link
Collaborator Author

wait, all the tests use this same task run 🤔

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 17, 2019
@bobcatfish
Copy link
Collaborator Author

curses!

@bobcatfish
Copy link
Collaborator Author 

I0418 00:36:41.683]     --- PASS: TestTaskRunTimeout/Timeout-0 (31.30s)
I0418 00:36:41.683]         init_test.go:100: Create namespace arendelle-8pf9m to deploy to
I0418 00:36:41.684]         init_test.go:112: Verify SA "default" is created in namespace "arendelle-8pf9m"
I0418 00:36:41.684]         timeout_test.go:191: Creating Task and TaskRun in namespace arendelle-8pf9m
I0418 00:36:41.684]         timeout_test.go:204: Waiting for TaskRun run-giraffe-10 in namespace arendelle-8pf9m to complete
I0418 00:36:41.684]         timeout_test.go:222: Deleting namespace arendelle-8pf9m

I think I've made some kind of variable scoping error 🤔

@bobcatfish
Classic iterator variable mistake 😅
b34d2ec 
All the tests were using the same run _still_
@bobcatfish
Copy link
Collaborator Author

This looks like it might be legit:

https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/771/pull-tekton-pipeline-integration-tests/1118981945099816962/

/test pull-tekton-pipeline-integration-tests

This was referenced Apr 18, 2019
Closed
Closed
@tekton-robot
Copy link
Collaborator

@bobcatfish: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-tekton-pipeline-integration-tests 8df78a2 link /test pull-tekton-pipeline-integration-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@bobcatfish bobcatfish mentioned this pull request Apr 18, 2019
Closed
@bobcatfish
Copy link
Collaborator Author

Thanks to these 2 most recent failures:

I think I know what's going on! The timeout handler and the reconciler do not agree about when a timeout has happened, more details in #731 (comment)

@bobcatfish bobcatfish closed this Apr 18, 2019
bobcatfish added a commit to bobcatfish/pipeline that referenced this pull request Apr 22, 2019
@bobcatfish
DO NOT MERGE ME - run timeout test 10 times
357a515 
To try to be sure that we've fixed the problem, I want to re-reate the
conditions from tektoncd#771 which seemed to be pretty good at re-producing the
problem.
bobcatfish added a commit to bobcatfish/pipeline that referenced this pull request Apr 22, 2019
@bobcatfish
DO NOT MERGE ME - run timeout test 10 times
68b8d93 
To try to be sure that we've fixed the problem, I want to re-reate the
conditions from tektoncd#771 which seemed to be pretty good at re-producing the
problem.
@bobcatfish bobcatfish mentioned this pull request Apr 22, 2019
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlorenc dlorenc Awaiting requested review from dlorenc

@vdemeester vdemeester Awaiting requested review from vdemeester

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bobcatfish @tekton-robot @googlebot