E2E: Simplify smoke and extended tests #2871

tanya-borisova · 2022-11-14T14:48:46Z

Resolves #2869

Refactor selectors in our E2E tests so that we have 3 groups of tests:

Smoke tests that need to pass before a PR is merged
Extended tests that will run on each push to main
Performance tests that will run nightly

How this was done

Added extended_aad and shared_services tests to extended selector so that they run on /test-extended
Remove /test-shared-services and /test-extended-aad from PR bot (they can still be ran separately using a custom selector) as well as Makefile and devcontainer
Add a fixture for Workspace Services
Move all template-retrieving tests to e2e_tests/test_get_templates.py to simplify folder structure a bit
Change selectors so that extended tests run on every push to main and on nightly build, and performance tests run on nightly build (maybe we should consider running it weekly?)

github-actions · 2022-11-14T14:50:00Z

Unit Test Results

0 tests 0 ✔️ 0s ⏱️
0 suites 0 💤
0 files 0 ❌

Results for commit 67ff46f.

♻️ This comment has been updated with latest results.

…microsoft/AzureTRE into tborisova/simplify-smoke-extended-tests

tamirkamara

Requesting changes as I don't want this PR merged before we can address my main concern: running more tests on PRs is very likely to make them harder to merge given the instability we see in Azure provisioning resources.

tamirkamara · 2022-11-15T06:30:29Z

e2e_tests/test_shared_services.py

@@ -91,6 +92,7 @@ async def test_patch_firewall(verify):
 ]


+@pytest.mark.extended


this currently fails 100% of the time on the main branch and is very likely to fail on repeat runs at some point in PR environments.

I was working on this another day, I managed to get Gitea deployed but I'm going to try delete it and re-deploy a couple more times. If it works, I'd like to leave it in

Yeah it still seems to be failing with the same problem "secret being recovered and can't be deleted". I will mark this test as skipped for now.

tamirkamara · 2022-11-15T06:31:35Z

e2e_tests/test_shared_services.py

@@ -9,6 +9,7 @@
 LOGGER = logging.getLogger(__name__)


+@pytest.mark.extended


Since almost every other service does an internal update on the firewall, do we even need it?

Good point, I think we can remove this one

tamirkamara · 2022-11-15T06:38:58Z

.devcontainer/devcontainer.json

@@ -89,36 +89,6 @@
            "false"
          ]
        },
-        {
-          "name": "E2E Extended AAD",


How does one debug a specific test?

You couldn't do that before either though? There is still a target for E2E Extended tests, there's just no separate target for AAD.

tamirkamara · 2022-11-15T06:40:45Z

.github/workflows/pr_comment_bot.yml

-      needs.pr_comment.outputs.command == 'run-tests-extended' ||
-      needs.pr_comment.outputs.command == 'run-tests-extended-aad' ||
-      needs.pr_comment.outputs.command == 'run-tests-shared-services'
+      needs.pr_comment.outputs.command == 'run-tests-extended'


Shouldn't the command be still available for cases where developers know what they want to test?
For example, if I change a shared service being tested I will call the run-shared-services without any of the workspace level stuff?

sort of agree - however now that workspaces + services can update those shared services, and do, there's also a case to be made that we should be running all the tests as we need to test the pipeline update from a workspace -> shared service?

Shouldn't the command be still available for cases where developers know what they want to test?

I think a more future-proof way to do it is would be to run a workflow with a custom selector. This way we can easily extend selectors without having to update PR bot each time.

there's also a case to be made that we should be running all the tests as we need to test the pipeline update from a workspace -> shared service?

Well other bundles only really update firewall, and that will be tested by deploying said bundles. I don't think we would need to test shared services each time

tanya-borisova · 2022-11-15T11:37:43Z

@tamirkamara

running more tests on PRs is very likely to make them harder to merge given the instability we see in Azure provisioning resources

This doesn't add any more tests that are required for a PR to pass though. Only running smoke tests is required. Given that running / test-extended is optional anyway, and we can run a custom subset of tests using a selector, is this a big concern?

tamirkamara · 2022-11-15T12:57:23Z

This doesn't add any more tests that are required for a PR to pass though. Only running smoke tests is required. Given that running / test-extended is optional anyway, and we can run a custom subset of tests using a selector, is this a big concern?

Granted - the test one hasn't changed much but I meant the extended one which is our "baseline" for the system to be functional within the PR validation.
Now extended becomes everything: gitea, firewall patch (and nexus, etc. in the future), two workspace flavors, guacamole (times 2), 2 types of VMs (linux and windows).
The question here is do we really want to do that? When we discussed this last (some months ago) we agreed to keep a balance between what runs in PR and what runs in a nightly test.
At any case, this is going to cause much more frictions to get PRs to merge and I'm not sure it's a good idea for now.

As for the custom selector, it's not relevant for many PRs as the bot doesn't (and shouldn't) expose that functionality. You must have a branch in this repo to make use of it.
Also, using a selector doesn't make the process "simpler" IMO.

tanya-borisova · 2022-11-15T14:55:57Z

@tamirkamara I agree that a balance should be kept, my intention was mostly bringing all these additional things to extended set of tests and only mandate to run it nightly.
Is there maybe a case of bringing some tests that were previously in extended test into smoke, to really make it, as you say, baseline for the system to be functional.

Alternatively we could create a new selector, call it baseline and only add tests there that are absolutely essential for the PR to pass. (Though wouldn't there be overlap with smoke, would we need both?)

Let's discuss on the morning call tomorrow?

tanya-borisova · 2022-11-16T09:51:51Z

We concluded that before this is useful, we should try and see if we can allow running tests with custom selector from PR bot, securely. I'm going to move it to draft for now

tanya-borisova added 9 commits November 11, 2022 14:45

Only leave extended and smoke main pytest targets

3e5bfbf

bring back deleted file

ad08fb3

Move test data into data/ folder

52d99cd

Refactor tests that get templates and move them all into one file

8cfc739

Add shared service tests into extended selector

b88d894

wip

92dd814

Add endopoint logging for debugging

7f2f6b2

Fix extended marks

61feaea

Merge branch 'main' into tborisova/simplify-smoke-extended-tests

45a9ba3

tanya-borisova added 3 commits November 14, 2022 14:52

Run performance tests nightly

b5a7df4

Merge branch 'tborisova/simplify-smoke-extended-tests' of github.com:…

5afc031

…microsoft/AzureTRE into tborisova/simplify-smoke-extended-tests

fix getting admin token

d0ee678

tanya-borisova marked this pull request as ready for review November 14, 2022 16:48

tamirkamara requested changes Nov 15, 2022

View reviewed changes

Merge branch 'main' into tborisova/simplify-smoke-extended-tests

8b0c374

Merge branch 'main' into tborisova/simplify-smoke-extended-tests

67ff46f

tanya-borisova marked this pull request as draft November 16, 2022 09:52

tanya-borisova added the blocked Cannot progress at present label Nov 16, 2022

tanya-borisova closed this Dec 8, 2022

marrobi deleted the tborisova/simplify-smoke-extended-tests branch March 14, 2024 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2E: Simplify smoke and extended tests #2871

E2E: Simplify smoke and extended tests #2871

tanya-borisova commented Nov 14, 2022 •

edited

Loading

github-actions bot commented Nov 14, 2022 •

edited

Loading

tamirkamara left a comment

tamirkamara Nov 15, 2022

tanya-borisova Nov 15, 2022

tanya-borisova Nov 15, 2022

tamirkamara Nov 15, 2022

tanya-borisova Nov 15, 2022

tamirkamara Nov 15, 2022

tanya-borisova Nov 15, 2022 •

edited

Loading

tamirkamara Nov 15, 2022

damoodamoo Nov 15, 2022

tanya-borisova Nov 15, 2022 •

edited

Loading

tanya-borisova commented Nov 15, 2022

tamirkamara commented Nov 15, 2022

tanya-borisova commented Nov 15, 2022

tanya-borisova commented Nov 16, 2022

		@@ -91,6 +92,7 @@ async def test_patch_firewall(verify):
		]


		@pytest.mark.extended

		@@ -9,6 +9,7 @@
		LOGGER = logging.getLogger(__name__)


		@pytest.mark.extended

E2E: Simplify smoke and extended tests #2871

E2E: Simplify smoke and extended tests #2871

Conversation

tanya-borisova commented Nov 14, 2022 • edited Loading

Resolves #2869

How this was done

github-actions bot commented Nov 14, 2022 • edited Loading

Unit Test Results

tamirkamara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanya-borisova Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanya-borisova Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

tanya-borisova commented Nov 15, 2022

tamirkamara commented Nov 15, 2022

tanya-borisova commented Nov 15, 2022

tanya-borisova commented Nov 16, 2022

tanya-borisova commented Nov 14, 2022 •

edited

Loading

github-actions bot commented Nov 14, 2022 •

edited

Loading

tanya-borisova Nov 15, 2022 •

edited

Loading

tanya-borisova Nov 15, 2022 •

edited

Loading