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

Remove unnecessary assertion in test #5013

Merged
merged 1 commit into from
Mar 18, 2016

Conversation

MCGallaspy
Copy link
Contributor

Summary

Seems to have been added by a revert-revert, however
it seems to be the wrong assertion to make in this case.
Causes flakiness if the modal is dismissed first.

Seems to have been added by a revert-revert, however
it seems to be the wrong assertion to make in this case.
Causes flakiness if the modal is dismissed *first*.
@MCGallaspy MCGallaspy added this to the 0.16.0 milestone Mar 17, 2016
@66eli77
Copy link
Contributor

66eli77 commented Mar 17, 2016

it should not cause any problem if modal is dismissed first because https://github.com/learningequality/ka-lite/blob/master/kalite/distributed/static/js/distributed/user/views.js#L101
just hide the model, means the test can still find the elements in the DOM and evaluate the color. and if the color is red, we should raise an exception.
I just quickly ran the test and didn't encounter any issue.
Did you actually encounter the flakiness issue? @MCGallaspy

@MCGallaspy
Copy link
Contributor Author

Did you actually encounter the flakiness issue?

Yes, I did.

just hide the model, means the test can still find the elements in the DOM and evaluate the color. and if the color is red, we should raise an exception.

Just because you can do it, should we? Is it a useful assertion to make, if the modal will also just be hidden anyway?

Edit: for flakiness, see the test results on this PR: #5010

@66eli77
Copy link
Contributor

66eli77 commented Mar 18, 2016

In theory, there should not be issue caused by this assertion.
the assertion just make sure when the modal isn't hidden, it's not because of wrong password or username.

@66eli77
Copy link
Contributor

66eli77 commented Mar 18, 2016

ok, I find the reason for the flaky issue
https://github.com/learningequality/ka-lite/blob/master/kalite/distributed/static/js/distributed/bundle_modules/homepage.js#L24
the modal does get removed from the DOM once superuser is successfully created

66eli77 added a commit that referenced this pull request Mar 18, 2016
Remove unnecessary assertion in test
@66eli77 66eli77 merged commit 6d6f65c into learningequality:0.16.x Mar 18, 2016
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.

3 participants