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): improve container start up error handling #7283

Merged
merged 4 commits into from
May 28, 2024

Conversation

odockal
Copy link
Contributor

@odockal odockal commented May 22, 2024

What does this PR do?

Improves reporting of the error during container start up in e2e tests.

Screenshot / video of UI

What issues does this PR fix or reference?

#7276

How to test this PR?

If container start up fails in the e2e tests, the error would be thrown.

Quite a lot to set a container to fail, you can add some custom code to mess up container start up ie. in run-image-page.ts file, add this into line 85:

      await this.activateTab('Security');
      const secPlaceholder = this.page.getByPlaceholder('Enter a security option');
      await secPlaceholder.fill('notreallyasecurityoption');

Then adjust package.json to run only container-smoke test, to make is faster, run
yarn test:e2e.
Test should fail with assumed error shown in the assertion output.

  • 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 May 22, 2024 17:58
@odockal odockal requested review from dgolovin and feloy and removed request for a team May 22, 2024 17:58
// 2. Opening Containers page with new container on it
// 3. staying on the run image page with an error
// 4. Starting a container without entrpoint or command creates a container, but it stays on Run Image Page without error
if ((await this.errorAlert.count()) > 0 && (await this.name.isVisible())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

just for readability I think it will be easier with intermediate variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I have changed the code a bit, added a wait for page to switch (or not) just to make sure to get right error in the line.

Signed-off-by: Ondrej Dockal <odockal@redhat.com>
import { BasePage } from './base-page';
import { ContainersPage } from './containers-page';

export interface ContainerInteractiveParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be here, it should be moved in one of the other files where interfaces are defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to core folder.

// 2. Opening Containers page with new container on it
// 3. staying on the run image page with an error
// 4. Starting a container without entrypoint or command creates a container, but it stays on Run Image Page without error
await waitWhile(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not replace this waitWhile() with await playExpect(this.name).toBeVisible() ? I'm not clear what happens if the field is not visible in your current implementation before the timer runs out, since the error is not propagated, it seems to me that the test would continue even though the field wouldn't be visible.

@@ -348,7 +355,8 @@ describe.skipIf(process.env.GITHUB_ACTIONS && process.env.RUNNER_OS === 'Linux')
const runImagePage = await imageDetailsPage.openRunImage();
await playExpect(runImagePage.heading).toContainText(backendImage);
await runImagePage.setCustomPortMapping(`${portsList[i]}:${portsList[i]}`);
const containersPage = await runImagePage.startContainer(containerNames[i]);
await runImagePage.startContainer(containerNames[i], containerStartParams);
const containersPage = new ContainersPage(page);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor

@cbr7 cbr7 left a comment

Choose a reason for hiding this comment

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

This new implementation is likely to lead to inconsistencies and I don't understand why the navigation was removed and now some methods that shouldn't are returning void.

… outcomes

Signed-off-by: Ondrej Dockal <odockal@redhat.com>
@odockal odockal requested a review from cbr7 May 23, 2024 15:15
Copy link
Contributor

@cbr7 cbr7 left a comment

Choose a reason for hiding this comment

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

LGTM

@odockal odockal enabled auto-merge (squash) May 28, 2024 16:34
@odockal odockal merged commit 65d14b4 into containers:main May 28, 2024
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.11.0 milestone May 28, 2024
cdrage pushed a commit to cdrage/podman-desktop that referenced this pull request Jun 19, 2024
)

* chore(tests): improve container start up error handling
- adjust startContainer method to be aligned with various outcomes

Signed-off-by: Ondrej Dockal <odockal@redhat.com>
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