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

feat(e2e): add e2e tests for stream disconnect, vhal methods [WD-11812] #42

Merged
merged 4 commits into from
Jul 2, 2024

Conversation

lorumic
Copy link
Contributor

@lorumic lorumic commented Jun 26, 2024

Done

  • Determine architecture automatically by querying the /nodes endpoint of AMS.
  • Add e2e tests for stream disconnect + done callback.
  • Add e2e tests for VHAL: vhalReady callback, getAllVhalPropConfigs, getVhalProperties, setVhalProperties methods.

Until #37 is merged, you can see the test passing here.

Fixes WD-11812

@lorumic lorumic requested a review from a team as a code owner June 26, 2024 09:25
Copy link
Contributor

@ajanon ajanon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small question: with this change, we would only be running e2e tests against AAOS 13, right?
Should we run these tests against multiple LXD images versions?
I'm not sure if it's too useful, but I would like to have other opinions.

@lorumic
Copy link
Contributor Author

lorumic commented Jun 26, 2024

Just one small question: with this change, we would only be running e2e tests against AAOS 13, right? Should we run these tests against multiple LXD images versions? I'm not sure if it's too useful, but I would like to have other opinions.

For now, I'm focusing on having a single image downloaded, and running tests on a session created from that one only. This is to make the tests run reasonably fast. If there is an actual need of running on different images, technically we could do that, however I don't know if it's in the scope of the e2e tests, as it seems more about "testing the images" rather than "testing the features of the Anbox streaming SDK end-to-end". However, this is just my point of view, so I'm happy to hear other opinions and change the workflow accordingly. :)

Copy link
Contributor

@ajanon ajanon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM then, thanks!

@morphis
Copy link
Collaborator

morphis commented Jun 26, 2024

For now, I'm focusing on having a single image downloaded, and running tests on a session created from that one only. This is to make the tests run reasonably fast. If there is an actual need of running on different images, technically we could do that, however I don't know if it's in the scope of the e2e tests, as it seems more about "testing the images" rather than "testing the features of the Anbox streaming SDK end-to-end". However, this is just my point of view, so I'm happy to hear other opinions and change the workflow accordingly. :)

There are quite a few nuances we do not necessarily can test with every image (e.g. I doubt we have virtual keyboard support for AAOS atm). I think you definitely want one per image variant (android, aaos and generic as well once we start officially supporting that).

js/tests/e2e/tests/vhal.spec.js Outdated Show resolved Hide resolved
@morphis
Copy link
Collaborator

morphis commented Jun 26, 2024

@lorumic I've merged #37, if you update the PR we should be able to get tests running now.

@lorumic
Copy link
Contributor Author

lorumic commented Jun 26, 2024

@morphis @ajanon

I'm laying the ground for when we'll need to test different features on different images. For this purpose, I tested locally the parallel creation of an AOSP-based app + session and AAOS-based ones. Thanks to the parallel execution, it should not take too much additional time - the test is still running, though, and we'll have to see how it behaves on the runner, since we'd also need to download both images there, unlike locally where they are already downloaded.

If that works well, I will run the join + disconnect stream test on the AOSP session, and the VHAL test (which includes join + disconnect anyway) on the AAOS one. Anything else, I'd like to keep as a future addition so as not to grow too much the scope of this task, which was planned to comprise only what is listed in the "Done" section of the PR description above.

Does that sound reasonable to you?

EDIT: I have pushed the changes already, but for some reason GitHub is not reflecting them on the PR after almost 30 minutes:

image

Anyway, I ran the job manually and you can compare the difference here:

With AOSP + AAOS it takes just 3 minutes more than with AAOS only. Not bad. :)

.github/workflows/e2e.yaml Show resolved Hide resolved
js/tests/e2e/tests/vhal.spec.js Outdated Show resolved Hide resolved
js/tests/e2e/tests/vhal.spec.js Outdated Show resolved Hide resolved
js/tests/e2e/tests/vhal.spec.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@morphis morphis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

js/tests/e2e/tests/vhal.spec.js Show resolved Hide resolved
Copy link
Contributor

@adglkh adglkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ajanon ajanon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@morphis
Copy link
Collaborator

morphis commented Jun 28, 2024

@lorumic I think we're blocked on #43 before we can merge this one?

@lorumic
Copy link
Contributor Author

lorumic commented Jun 28, 2024

@lorumic I think we're blocked on #43 before we can merge this one?

Yes indeed, I was hoping to test the outcome of #43 on this one before merging it.

@lorumic
Copy link
Contributor Author

lorumic commented Jul 2, 2024

As discussed with @edlerd, there are changes to the workflow in this PR which won't get picked up, making the tests still fail. I have another addition of e2e tests in the works (based on this one) which doesn't make changes to the workflow, so we can test the CI changes on that one once I open it. You can check that the tests for this branch are passing by looking at the fork run. So I'm merging this now.

@lorumic lorumic merged commit 9dead10 into canonical:main Jul 2, 2024
1 of 2 checks passed
@lorumic lorumic deleted the new-e2e-tests branch July 2, 2024 08:18
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.

4 participants