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

chore(tests): update extensions tests, pom and related functionality #6931

Merged
merged 4 commits into from
Apr 24, 2024

Conversation

odockal
Copy link
Contributor

@odockal odockal commented Apr 24, 2024

What does this PR do?

update extensions tests and pom to align with new Extensions pages.

Screenshot / video of UI

What issues does this PR fix or reference?

#6894

How to test this PR?

yarn test:e2e:extension

  • Tests are covering the bug fix or the new feature

Signed-off-by: Ondrej Dockal <odockal@redhat.com>
@odockal odockal requested review from benoitf and a team as code owners April 24, 2024 15:19
@odockal odockal requested review from dgolovin and cdrage and removed request for a team April 24, 2024 15:19
@@ -18,10 +18,10 @@

import type { Page } from '@playwright/test';

import { ExtensionPage } from './extension-page';
import { ExtensionDetailsPage } from './extension-details-page';
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we have this class in this repository ? AFAIK it should be in its own repository

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is here for some time already.
Was also an option to have them in their particular repos, altough, if you want to test multiple external resources, you would need to share POM. Ideally on once place and this repo seems like logical place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it should not be there at all. Podman Desktop has no knowledge on 3rd party extension

you may have different model page for each version of the extension so it can't be there

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you want to test multiple external resources, you would need to share POM

I don't get it there ? Nothing related to external extension should be there, only the things concerning the pages provided by Podman Desktop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, removing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well this is a change from the previous agreement as I understood it, previously it was discussed that extension tests should be in the extension repos, but that the framework part could be here so that we can use it in other tests also.

For example if we move the framework part in their respective extension repos it will be much harder to create an e2e test that could test other features that might also imply some functionality from an extension, like for example if you wanted an e2e test that would use functionality from AI Lab and bootc in one test.

Copy link
Contributor

@ScrewTSW ScrewTSW left a comment

Choose a reason for hiding this comment

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

LGTM

return new ExtensionDetailsPage(this.page, this.extensionName);
}

async disableExtension(): Promise<this> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these methods already exposed from extension-details-page.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extensions Details page and Extensions/ExtensionsCard Page are two distinguished pages with the same locators.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean yes, but if the functionality is identical it suggests that some kind of abstract class that both page object inherit should be used, there is no reason to have two different POM models implement literally identical functionalities.

Signed-off-by: Ondrej Dockal <odockal@redhat.com>
@odockal
Copy link
Contributor Author

odockal commented Apr 24, 2024

@cbr7 @ScrewTSW Changes done here are reflected in sso repo as wip pr. For you to see how it is gonna change at your extensions.

Signed-off-by: Ondrej Dockal <odockal@redhat.com>
Signed-off-by: Ondrej Dockal <odockal@redhat.com>
@odockal
Copy link
Contributor Author

odockal commented Apr 24, 2024

@benoitf I have noticed failing podman-extension tests. Fixed that by the latest commit.

@odockal odockal enabled auto-merge (squash) April 24, 2024 18:05
@odockal odockal merged commit b9c9f4d into containers:main Apr 24, 2024
8 checks passed
@benoitf
Copy link
Collaborator

benoitf commented Apr 24, 2024

FYI there is a typo in the last commit

const exntesionLabelName = 'podman';

exntesion vs extension

@podman-desktop-bot podman-desktop-bot added this to the 1.10.0 milestone Apr 24, 2024
@odockal
Copy link
Contributor Author

odockal commented Apr 24, 2024

FYI there is a typo in the last commit

`const exntesionLabelName = 'podman';``

exntesion vs extension

Uf, nice, glad it is variable name.

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.

None yet

5 participants