-
Notifications
You must be signed in to change notification settings - Fork 0
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 initial explorer test for poc validation #16
base: main
Are you sure you want to change the base?
Conversation
@@ -21,7 +21,7 @@ env: | |||
CLUSTER_ADMIN_PASSWORD_HASH: ${{ secrets.CLUSTERS_CONFIG_PASSWORD }} | |||
WEAVEWORKS_BOT_TOKEN: ${{ secrets.WEAVEWORKS_BOT_TOKEN }} | |||
ENTERPRISE_CHART_VERSION: ${{ inputs.chart_version }} | |||
DEFAULT_ENTERPRISE_CHART_VERSION: "0.31.0-9-gdae6755" | |||
DEFAULT_ENTERPRISE_CHART_VERSION: "0.38.0-4-gaa8c216" # release 0.38.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we eventually should have the latest release
self.page = page | ||
self.url = url | ||
|
||
def open(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function name here is so generic , you need to specify which page you need to open just to make it easy for any one else understand what the function does and search about it easily despicably if you have large testing framework, you can refer to policies page to see how we name the function in page object model pattern.
def open(self): | ||
self.page.get_by_role("link", name="Explorer").click() | ||
|
||
def search(self, term): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function name here is so generic , you need to specify which control exactly you need to get just to make it easy for any one else understand what the function does and search about it easily despicably if you have large testing framework, you can refer to policies page to see how we name the function in page object model pattern.
self.page.get_by_role("link", name="Explorer").click() | ||
|
||
def search(self, term): | ||
self.page.goto(f"{self.url}/explorer/query?descending=false&terms={term}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In automation testing you need to interact more with controls in the page instead of drop the URL easily in the address bar to make sure that this controls and fields respond correctly.
So in this case you need to catch the Filter& search icon in the page and select the value you need to Search by same as we do manually like in the below record
Explorer_Search.mp4
class Explorer: | ||
def __init__(self, page, url): | ||
self.page = page | ||
self.url = url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After fixing this line , the URL will not be needed.
self.page = login | ||
self.explorer_page = Explorer(self.page, self.URL) | ||
|
||
def test_search(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test function name here is so generic also , you need to specify what exactly you need to test, you can refer to this file to see how we name the testing functions in page object model pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to create a Folder for each section we add tests for, we just Append our tests files to the test_weave_gitops_enterprise and name the file a name refers to what exactly it tests so in this case the file name will be test_explorer_search.py
Thanks for your review @taghreed86! agree that the added test might not stick to good practices which i think aligns with this part of my feedback given that I represent the case of a developer with limited exposure as QA, do you think that it makes sense to add some basic references, for example as part a contributing file in this repo to setup some baseline expectations to add new tests? see for example https://github.com/weaveworks/weave-gitops-enterprise/blob/main/CONTRIBUTING.md if you think that makes sense, i will wait to that content to be produced to read it and apply the suggestions to the PR based on it. |
@enekofb I don't mind to add guide for how to add automation tests but I preferred to add it as a notion page instead of github repo doc like that page |
Thanks @taghreed86, here the ticket weaveworks/weave-gitops-enterprise#3728 I rather wait for the doc to exists so we use this PR also for validation of that guidance: i guess the value of this validation is that we have everything we need for others to jump in. |
Part of weaveworks/weave-gitops-enterprise#3711
This PR adds explorer test case to validate the PoC with the following feedback:
Get Started
I had to setup my environment using the following commands
We should consider help other folks with the getting started as might be in the same scenario
Setup Environment
Environment Setup
.Add Test
CONTRIBUTING.md
doc with guidance on a) how to extend the suite, b) how to do a good acceptance test, c) info about maintainabilityOthers
To add the test, given i was not familiar with playwright, i used generators
It would be good to add it as part of the readme