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

Added a sleep for onscreen rendered tests #1393

Merged
merged 1 commit into from Jun 17, 2015

Conversation

@chaosphere2112
Copy link
Contributor

@chaosphere2112 chaosphere2112 commented Jun 15, 2015

This should help generalize what @durack1 did in #1341 (adding a sleep to handle the race condition with resized windows). The root of the issue is #1148; I have some tests that resize the window (to make sure everything is well-behaved), and because of #1148 they have to be rendered onscreen. This makes them run slowly, which looks like it adds a race condition– see intermittent failures of the resize manager and background color tests. This makes it so the base class checks if the window is set to render offscreen; if it isn't, it will sleep.

@durack1
Copy link
Member

@durack1 durack1 commented Jun 15, 2015

@chaosphere2112 it doesn't seem like you triggered the buildbot tests - you might have to push this PR as a new branch on UV-CDAT/master to get these to fire

@jbeezley
Copy link

@jbeezley jbeezley commented Jun 16, 2015

Yeah, you need to push this to the main repo for the tests to run. Anyway, this change gets a 👍 from me.

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Jun 16, 2015

😕 I can't seem to push to this repo. I'm set up with private key/SSH, and it tells me that I can't push, and that I should try using HTTPS instead. That's an issue for me, because I have 2FA set up, and it is a giant pain making that work. Any ideas?

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Jun 16, 2015

Never mind, looks like I figured out how to do it over https...

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Jun 16, 2015

@jbeezley Looks like the garant buildbot didn't get much of anywhere on this... any ideas?

@jbeezley
Copy link

@jbeezley jbeezley commented Jun 16, 2015

Weird the git clone failed:

error: RPC failed; result=6, HTTP code = 0
fatal: The remote end hung up unexpectedly

I'll restart it.

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Jun 17, 2015

@doutriaux1 @jbeezley @aashish24 Anything preventing this from being merged?

@jbeezley
Copy link

@jbeezley jbeezley commented Jun 17, 2015

No, looks good. :shipit:

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jun 17, 2015

@durack1 was telling me yesterday that sleep(1) might be better than sleep(2). But if you're cool with sleep(2) I can approve

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Jun 17, 2015

This should only happen for a few things, and I'd rather be over-cautious than under. Go ahead and approve.

doutriaux1 added a commit that referenced this issue Jun 17, 2015
Added a sleep for onscreen rendered tests
@doutriaux1 doutriaux1 merged commit 209569b into CDAT:master Jun 17, 2015
4 checks passed
@durack1
Copy link
Member

@durack1 durack1 commented Jun 17, 2015

@doutriaux1 @chaosphere2112 just FYI, I did pull my sleep(1) back out of #1341 and sure enough the PR failed again due to this race condition so I might have to rebase #1341 to master merging #1393 changes to bring things back to passing..

Just FYI, the single file kludge sleep(1) is now in the master branch - this should probably be reverted as this merged PR #1393 should have fixed that issue..

@durack1
Copy link
Member

@durack1 durack1 commented Jun 17, 2015

@doutriaux1 @chaosphere2112 this should be fixed in updated PR #1341 with these edits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants