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(test,ci): fix broken e2e test execution #5003

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

ScrewTSW
Copy link
Contributor

@ScrewTSW ScrewTSW commented Nov 27, 2023

What does this PR do?

  • Fixes missing playwright dependencies installation

Screenshot/screencast of this PR

What issues does this PR fix or reference?

#4983
#5032

How to test this PR?

  • yarn --frozen-lockfile
  • yarn run test:e2e:smoke

@ScrewTSW ScrewTSW requested review from benoitf and a team as code owners November 27, 2023 13:53
@ScrewTSW ScrewTSW requested review from jeffmaury and feloy and removed request for a team November 27, 2023 13:53
Copy link
Contributor

@jeffmaury jeffmaury 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
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

it only needs to be done once, so it should not be done at each run of the test command

@odockal
Copy link
Contributor

odockal commented Nov 27, 2023

@benoitf It does not force overwrite the artifacts, so if it is called twice, it does not do anything if already up to date.
Before it was part of yarn install, now it would be part of yarn test:e2e:XXX, which is truly called more than once when running tests, but it is better, imho, than adding this steps to the every workflow.

@benoitf
Copy link
Collaborator

benoitf commented Nov 27, 2023

so if it is called twice, it does not do anything if already up to date
yes but it's still not something that we should do there

move it to postinstall where it means it should be only done 'once' when dependencies are updated/changed

@odockal
Copy link
Contributor

odockal commented Nov 27, 2023

@benoitf Nice, neat.

package.json Show resolved Hide resolved
@ScrewTSW ScrewTSW force-pushed the 4983-fixup-e2e-smoke-run-failure branch from 09d4934 to af0d4fa Compare November 27, 2023 15:55
package.json Outdated Show resolved Hide resolved
@odockal
Copy link
Contributor

odockal commented Nov 28, 2023

It is all messy. I have created new issue that this PR directly addresses. It is #5032.
#4983 which I though was to be addressed by this PR will probably not be fixed by this.

package.json Outdated Show resolved Hide resolved
@ScrewTSW ScrewTSW force-pushed the 4983-fixup-e2e-smoke-run-failure branch 3 times, most recently from 0ef340a to 56a56f8 Compare November 28, 2023 23:28
Signed-off-by: Tibor Dancs <tdancs@redhat.com>
@ScrewTSW ScrewTSW force-pushed the 4983-fixup-e2e-smoke-run-failure branch from 56a56f8 to c2f179c Compare November 28, 2023 23:29
@ScrewTSW ScrewTSW merged commit 67c96de into containers:main Nov 29, 2023
13 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.6.0 milestone Nov 29, 2023
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