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 initial explorer test for poc validation #16

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

enekofb
Copy link
Collaborator

@enekofb enekofb commented Dec 12, 2023

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

eneko-setup:
	brew install python
	pip3 install -U pytest
	pip3 install pytest-playwright
	playwright install

We should consider help other folks with the getting started as might be in the same scenario

Setup Environment

  • I did setup my environment using cli bootstrapping cause it was easier for me than following the steps in Environment Setup.
  • We should try to automate through Makefile so CI and Developer uses the same logic

Add Test

  • I was able to add a simple test based on the existing examples.
  • However, i had to do some decisions like where to put my test, etc ... We could add a CONTRIBUTING.md doc with guidance on a) how to extend the suite, b) how to do a good acceptance test, c) info about maintainability

Others

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

@@ -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
Copy link
Collaborator Author

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

@enekofb enekofb marked this pull request as ready for review December 12, 2023 17:16
self.page = page
self.url = url

def open(self):
Copy link
Contributor

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):
Copy link
Contributor

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}")
Copy link
Contributor

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
Copy link
Contributor

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):
Copy link
Contributor

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.

Copy link
Contributor

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

@enekofb
Copy link
Collaborator Author

enekofb commented Dec 14, 2023

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

Screenshot 2023-12-14 at 10 30 42

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.

@taghreed86
Copy link
Contributor

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

Screenshot 2023-12-14 at 10 30 42

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
so in that case you need to submit a new issue and assign it to me to work on it,
and in that time you can work on my review comments especially I added it explained in details just to save time and finalize the PR and merge it to main while I'm working on the guide doc or if you preferred to wait ..it's up to you.

@enekofb
Copy link
Collaborator Author

enekofb commented Dec 14, 2023

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
Screenshot 2023-12-14 at 10 30 42
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 so in that case you need to submit a new issue and assign it to me to work on it, and in that time you can work on my review comments especially I added it explained in details just to save time and finalize the PR and merge it to main while I'm working on the guide doc or if you preferred to wait ..it's up to you.

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.

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.

2 participants