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 token ui tests #955

Merged
merged 5 commits into from
Jun 19, 2019
Merged

Fix token ui tests #955

merged 5 commits into from
Jun 19, 2019

Conversation

greenape
Copy link
Member

Wait for permissions to be loaded before finishing processing click events on top level checkboxes - fixes sporadic fail on CI.

(Also fixes permissions boxes not showing up when first adding a new server)

Wait for permissions to be loaded before finishing processing click events on top level checkboxes
@greenape greenape added bug Something isn't working FlowAuth Issues related to FlowAuth tests Issues relating to tests CI Issues related to continuous integration & circleci labels Jun 19, 2019
@greenape greenape added the ready-to-merge Label indicating a PR is OK to automerge label Jun 19, 2019
@@ -14,95 +14,96 @@ describe("Server management", function() {
it("Add server name with space", function() {
cy.get("#new").click();
// adding username with space
cy.get("#name").type("Server ");
cy.get("#name").type("Server ", { force: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of force: true here (and below)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Works around cypress trying to scroll the thing into view, but it ending up hidden because their scrolling algorithm is bad. This types (or whatever) into the box even if it isn't visible - I'd add a comment to this effect, but we end up having to do it pretty much universally across the tests, so is already mentioned elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I was going to say the same thing. Would be nice to have a comment but since it's everywhere it's already fairly visible (just not entirely clear why if you don't see that comment). A bit annoying that we have to do this everywhere but seems to be necessary. Out of interest, is this a known bug that they are aware of and that might be fixed some time in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, see cypress-io/cypress#871

They are apparently working on it (or were in December last year).

cy.contains("#name-helper-text").should("not.exist");
});
it("Add server name more than 120 characters", function() {
cy.get("#new").click();
//adding username
cy.get("#name").type(
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
Copy link
Contributor

Choose a reason for hiding this comment

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

In a recent PR you had a nice concise way of writing this (something like "a".repeat(121) or similar). Do you want to change it here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh yes, in #864
Yeah, might as well throw that in here as well.

@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #955 into master will increase coverage by 1.39%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #955      +/-   ##
==========================================
+ Coverage   89.82%   91.22%   +1.39%     
==========================================
  Files          14       13       -1     
  Lines        1278     1071     -207     
  Branches       56       35      -21     
==========================================
- Hits         1148      977     -171     
+ Misses        123       89      -34     
+ Partials        7        5       -2
Flag Coverage Δ
#flowapi_unit_tests 77.9% <ø> (ø) ⬆️
#flowauth_unit_tests 94.86% <ø> (ø) ⬆️
#flowclient_unit_tests ?
#flowkit_jwt_generator_unit_tests 100% <ø> (ø) ⬆️
Impacted Files Coverage Δ
flowclient/flowclient/client.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20ffd70...f77ee0d. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #955 into master will increase coverage by 1.39%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #955      +/-   ##
==========================================
+ Coverage   89.82%   91.22%   +1.39%     
==========================================
  Files          14       13       -1     
  Lines        1278     1071     -207     
  Branches       56       35      -21     
==========================================
- Hits         1148      977     -171     
+ Misses        123       89      -34     
+ Partials        7        5       -2
Flag Coverage Δ
#flowapi_unit_tests 77.9% <ø> (ø) ⬆️
#flowauth_unit_tests 94.86% <ø> (ø) ⬆️
#flowclient_unit_tests ?
#flowkit_jwt_generator_unit_tests 100% <ø> (ø) ⬆️
Impacted Files Coverage Δ
flowclient/flowclient/client.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20ffd70...f77ee0d. Read the comment docs.

Copy link
Contributor

@maxalbert maxalbert left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell. 👍

@greenape greenape merged commit 0b9fac4 into master Jun 19, 2019
@greenape greenape deleted the fix-token-ui-tests branch June 19, 2019 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CI Issues related to continuous integration & circleci FlowAuth Issues related to FlowAuth ready-to-merge Label indicating a PR is OK to automerge tests Issues relating to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants