Expand UI test #1027
Expand UI test #1027
Conversation
There was a problem hiding this comment.
I won't be able to approve any PRs that expand on this type of testing. It is too early to be tightly coupling these tests to individual DOM elements on the page. These tests will need to be fixed very often as we iterate on the UI. In addition, they don't seem to validate that the UI is working correctly end-to-end.
Let's schedule some time to talk about how we want to do this type of testing as a team.
| AddAppHeader: webDriver.FindByXPath(`//*[@id="app"]//nav//h2`), | ||
| AppName: webDriver.FindByID("name"), | ||
| AppNamespace: webDriver.FindByID("namespace"), | ||
| AppRepoURL: webDriver.FindByID("url"), |
There was a problem hiding this comment.
This will be very brittle.
There was a problem hiding this comment.
Do you plan to change the ID placement a lot? Or will the overall UI outlook change too much?
There was a problem hiding this comment.
Changed the xpaths to be as unique as possible
test/acceptance/test/ui_tests.go
Outdated
| }) | ||
|
|
||
| By("Then I should see Authentication Button pop up", func() { | ||
| _ = Expect(addAppPage.AuthenticationButton.Visible()).To(BeTrue()) |
There was a problem hiding this comment.
What does .Visible() check for? The presence of the element in the DOM?
There was a problem hiding this comment.
No, if the element is visible to the user. As soon as you hit Add Application Button, the Github Authentication button pops up. The point of this function is to verify the button actually pops up in the UI
There was a problem hiding this comment.
Visibility can take lots of forms. I can change it with css via display: none, visibility: none, opacity: 0, or remove it from the DOM entirely.
There was a problem hiding this comment.
I think the use of visible matcher can make condition more understandable.
Expect(addAppPage.AuthenticationButton).Should(BeVisible())
The matcher passes if the selection refers to element is displayed on the page.
Incase the element is not present in the DOM the webdriver throws an error straightaway.
There was a problem hiding this comment.
I'm removing this for now since it requires a whole workaround and is causing weird failures. Although, @saeedfazal, I'll use your suggestion when we have a workaround for the gh auth flow.
Co-authored-by: Jordan Pellizzari <jordan@weave.works>
There was a problem hiding this comment.
I won't hold up merging for now, but @rokshana-b @iahmad9 @JamWils let's set up some time to discuss this as a team.
| @@ -127,6 +127,7 @@ jobs: | |||
| github-token: ${{ secrets.WEAVE_GITOPS_TEST_WEAVEWORKS_WEAVE_GITOPS_BOT_TOKEN }} | |||
| gitlab-key: ${{ secrets.GITLAB_KEY }} | |||
| gitlab-token: ${{ secrets.GITLAB_TOKEN }} | |||
| artifacts-base-dir: "/tmp/gitops-test" | |||
There was a problem hiding this comment.
This value is used in a whole bunch of jobs. I think it should be moved to the environment for the workflow:
name: run tests
env:
ARTIFACTS_BASE_DIR: "/tmp/gitops-test"
jobs:
...
artifacts-base-dir: ${{ env.ARTIFACTS_BASE_DIR }}
...
Closes: #903
What changed?
httpsWhy?
To add test coverage for UI
How did you test it?
Tested locally and on CI
Release notes
N/A
Documentation Changes
N/A