-
Notifications
You must be signed in to change notification settings - Fork 389
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
UI Automation Tests for LLM Studio #561
Conversation
Makefile
Outdated
@@ -142,4 +172,4 @@ build-doc-locally: # Bundles your website into static files for production | |||
cd documentation && npm run build | |||
|
|||
serve-doc-locally: # Serves the built website locally | |||
cd documentation && npm run serve | |||
cd documentation && npm run serve |
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.
missing new line
Pipfile
Outdated
pytest-bdd = "==6.1.1" | ||
playwright = "==1.40.0" | ||
hac-playwright = { file = "http://h2o-public-test-data.s3.amazonaws.com/e2e-testing/hac_playwright-1.40.0-py3-none-any.whl" } | ||
pytest-base-url = "==2.0.0" |
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.
missing new line
tests/ui/llm_studio.feature
Outdated
And I run the experiment | ||
Then I should see the test-experiment should finish successfully | ||
When I delete experiment test-experiment | ||
Then I should not see the experiment test-experiment |
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.
missing new line
tests/ui/utils.py
Outdated
# If the heading is present, click the "I agree" button | ||
page.get_by_role("button", name="I agree").click() | ||
else: | ||
return 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.
- this method has None return type
- the return value is not used anywhere in the code
thus entire section could be removed:
else:
return 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.
I still have issues running the tests locally (make test-ui-local
), in particular it fails on page.goto(base_url)
with base_url
being an empty string.
tests/ui/llm_studio_page.py
Outdated
from hac_playwright.pages.base import BasePage | ||
from playwright.sync_api import expect | ||
|
||
CLOUD_FILESYSTEM_PATH = "/home/llmstudio/mount/data/user/" |
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.
As discussed, this won't work for local tests.
Probably an env variable in the makefile together with an if-else condition will do the job (unless there's a more direct way of detecting running on the cloud):
test-ui-local:
$LOCAL_TEST=true \
(PW_DEBUG) $(PIPENV) run pytest \ ...
Within the python code:
if "LOCAL_TEST" in os.environ
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.
I can create an env variable to store the local directory to run the tests locally. WDYT? eg:
export FILESYSTEM_PATH=<path to local filesystem>
you can manually export this env var to add path which shall trump $CLOUD_FILESYSTEM_PATH
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.
There may be other places where one may need to distinguish between local vs. cloud (e.g. disable login for local), having a flag may be more flexible.
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.
makes sense yess
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 general, Page Objects should not contain any test logic and test data, like specific paths, names etc.
It's just an abstraction layer to express your tests steps.
So all paths, names belong to test cases/fixtures only.
tests/ui/llm_studio.feature
Outdated
Then I should see the test-experiment should finish successfully | ||
When I delete experiment test-experiment | ||
Then I should not see the experiment test-experiment | ||
|
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.
Line 25 contains spaces " ", but it should be just new line character at the end of 24
tests/ui/llm_studio_page.py
Outdated
from hac_playwright.pages.base import BasePage | ||
from playwright.sync_api import expect | ||
|
||
CLOUD_FILESYSTEM_PATH = "/home/llmstudio/mount/data/user/" |
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 general, Page Objects should not contain any test logic and test data, like specific paths, names etc.
It's just an abstraction layer to express your tests steps.
So all paths, names belong to test cases/fixtures only.
tests/ui/llm_studio_page.py
Outdated
|
||
def view_experiment_page(self): | ||
locator = self.page.get_by_role("button", name="View experiments") | ||
locator.click() |
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.
- self.page.get_by_role("button", name="View experiments").click()
- shouldn't we wait for the experiments page to be displayed?
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 dont have to
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.
Is it failing for you?
tests/ui/llm_studio_page.py
Outdated
self.page.wait_for_timeout(3000) | ||
|
||
def experiment_name(self, name: str): | ||
locator = self.get_by_test_id(self.EXPERIMENT_NAME_SELECTOR) |
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.
self.get_by_test_id(self.EXPERIMENT_NAME_SELECTOR).fill(name)
tests/ui/llm_studio_page.py
Outdated
locator.fill(name) | ||
|
||
def llm_backbone(self, value: str): | ||
locator = self.page.get_by_role("combobox", name="LLM Backbone") |
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.
self.page.get_by_role("combobox", name="LLM Backbone").fill(value)
|
||
@when("I login to LLM Studio", target_fixture="llm_studio") | ||
def login_to_llm_studio(logger: logging.Logger, page: Page, base_url: str): | ||
okta_user = os.environ.get("OKTA_USER") |
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.
what if we deploy LLM Studio to GCP env with keycloak instead of Okta?
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 need account for that yes. Do you know how to achieve this?
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.
jenkins has secrets for cloud-qa and managed cloud
Pipfile
Outdated
@@ -68,3 +68,7 @@ types-pyyaml = ">=6.0" | |||
types-requests = ">=2.31" | |||
types-toml = ">=0.10" | |||
wheel = "==0.41.0" | |||
pytest-bdd = "==6.1.1" | |||
playwright = "==1.40.0" | |||
hac-playwright = { file = "http://h2o-public-test-data.s3.amazonaws.com/e2e-testing/hac_playwright-1.40.0-py3-none-any.whl" } |
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.
This is currently blocking using 1.40
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.
okay, installation the previous available version 1.38.0 instead
Makefile
Outdated
$(PIP) install pip --upgrade | ||
$(PIP) install "pipenv>=2023.11.15" |
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.
let's stick to what is on main branch, if not causing troubles.
Fixed pip,pipenv version and newer than your min. pipenv
|
||
.PHONY: setup-no-flash | ||
setup-no-flash: pipenv | ||
$(PIPENV) install --verbose --python $(PYTHON_VERSION) | ||
|
||
setup-ui: pipenv |
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.
Why does it need another make command, why is -dev not enough? It should get some coverage in the README.md or any other appropriate docs how the tests are to be used.
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.
Probably designed to be run on a remote machine connecting to another instance? Again, should get coverage in docs why it has been designed this way and how it is to be used.
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.
precisely, its designed to be able to run from a remote machine. I'll add documentation coverage
Pipfile
Outdated
@@ -68,3 +68,7 @@ types-pyyaml = ">=6.0" | |||
types-requests = ">=2.31" | |||
types-toml = ">=0.10" | |||
wheel = "==0.41.0" | |||
pytest-bdd = "==6.1.1" | |||
playwright = "==1.40.0" |
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.
afaik, playwright
is not needed when you already specify hac-playwright
. Please confirm.
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.
youre right, works without playwright
# Conflicts: # Pipfile.lock # requirements.txt
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.
Thanks a lot for adding the missing Ui tests!
PR looks go to me, we can address subsequent refinements in upcoming PRs.
How to run the tests
Test results are stored at
reports/junit_ui.xml