chore(publish): improve e2e tests #3580

namjul · 2022-09-23T17:51:01Z

The aim of this PR is to add tests for checking if assetPrefixes are applied. It includes some improvements that emerged out of figuring this out.

Some docs on how to add playwright tests are also part of the aim of this PR [[dendron://dendron.docs/pkg.nextjs-template.qa.test#e2e-testing-with-playwright]]

Pull Request Checklist

If you are a community contributor, copy and paste the PR template from Dendron Community PR Checklist and add it to the body of the pull request.

If you are a team member, copy and paste the template from Dendron Extended PR Checklist.

To copy the template, click on the Raw button on the gist to copy the plaintext version of the template to this PR.

kevinslin

asset prefix is set in the build step by writing to .env.production. [[dendron://dendron.docs/pkg.nextjs-template.ref.asset-prefix]]
for testing, we can write to .env.test. see https://nextjs.org/docs/basic-features/environment-variables#test-environment-variables

packages/nextjs-template/utils/links.ts

packages/nextjs-template/playwright.config.ts

This is a more ergonomic API and want to use that inside [[dendron://dendron.docs/pkg.nextjs-template.qa.test]].

… command This commit aims to allow passing arguments to the mentions command like so: `yarn cli:test:template:docker --update-snapshots`. It appears docker does not support positional arguments therefore prepending `$0` so that the first argument gets applied.

The issue is that add that env var results into css styles not being rendered and therefore changing the form of that result that we want to test with visual regression testing. Maybe we need to seperate VRT from e2e testing which appear to me to be actually not the same. VRT does not require interaction with the page.

Lets keep this how it was for now.

namjul · 2022-09-26T19:15:26Z

@kevinslin pinging you with the thing I am grabing after with which is writing the assetprefix test case. In c2c95e8 (#3580) I temporarily disabled that case so that our snapshots stay correct. Meaning when having NEXT_PUBLIC_ASSET_PREFIX set, certain styles won't get loaded because the assets path changes. The downstream effect is e.g. that links appear not anymore with the green coloring.
So we cannot set NEXT_PUBLIC_ASSET_PREFIX without also changing visual regression tests. A thought than came to me that maybe VRT can be seperated from other e2e tests because its not interactive. Then we could only set NEXT_PUBLIC_ASSET_PREFIX when not doing VRT.

The next question that would then come up is that we would only set NODE_ENV=test within one e2e part and not inside the VRT. But it feels then off that these two do not converge on the fact that one has NODE_ENV=test and the other not. But maybe I am just missing a better frame around this.

About how to do the seperation, I can think of having two directories with e2e accompanied by two test commands.

kevinslin · 2022-09-26T20:06:05Z

Then we could only set NEXT_PUBLIC_ASSET_PREFIX when not doing VRT.
The next question that would then come up is that we would only set NODE_ENV=test within one e2e part and not inside the VRT. But it feels then off that these two do not converge on the fact that one has NODE_ENV=test and the other not. But maybe I am just missing a better frame around this.

for VRT tests, can we test with assetPrefix not set ?

namjul · 2022-09-27T12:33:27Z

for VRT tests, can we test with assetPrefix not set ?

Further grasping of the issue made me conclude that the pivot point for seperation is when we build nextjs for all the tests in global-setup, which has to know of the env-vars we want to test. So we need to restructure the thing that we have two builds one where certain env-vars are set and other not and then run certain tests only withing one or the other build. This will complicate the setup and I hestiate to start this which is the reason I first lay this out here.

kevinslin · 2022-09-27T16:44:45Z

i think we ultimately do need two builds if we want to test this e2e.
something that should make this easier - we have a way of specifying where the build should be made, see [[dendron://private/task.2022.09.11.make-nextjs-build-more-customizable]]

can we do something like the following?

BUILD_DIR=general-build next build
BUILD_DIR=assetPrefix-build next build

and when running the test, run the test in the right build director?

BUILD_DIRS = [...]
BUILD_DIRS.map {
    await cli.nextBuild([path.join(__dirname, "..", BUILD_DIR)]);
}

ideally what we want:

[regular build, simple build].forEach(build => {
    runAllCommonTest(build);
    runBuildSpecificTest(build)
})

- general.spec.ts
// tests that are only for assetPrefix
- scenario.assetPrefix.spec.ts

…test

This commit aims to allow `yarn run ci:test:template:docker -u` or any other multiple arguments appendix.

The cause of the changes is an update from playwright and its browser dependencies.

namjul · 2022-09-29T12:54:38Z

@kevinslin found a way without having multiple build steps 54cf881 (#3580).

namjul · 2022-09-29T12:57:36Z

@kevinslin I found an issue where the logo will not get loaded when providing a asset-prefix which will not get checked for here

dendron/packages/common-all/src/utils/index.ts

Line 856 in 6e3d250

static getSiteLogoUrl(config: IntermediateDendronConfig): string | undefined {

.

Shall I continue withing this PR fixing that and adding missing checks for correctly applies asset-prefix?

kevinslin · 2022-09-29T13:15:12Z

@kevinslin I found an issue where the logo will not get loaded when providing a asset-prefix which will not get checked for here

dendron/packages/common-all/src/utils/index.ts

Line 856 in 6e3d250

static getSiteLogoUrl(config: IntermediateDendronConfig): string | undefined {

.

Shall I continue withing this PR fixing that and adding missing checks for correctly applies asset-prefix?

continue with the pr, we'll fix the logo in a separate pr

kevinslin

looks good - nitpick on typo

kevinslin · 2022-09-29T18:49:01Z

packages/nextjs-template/e2e/general.spec.ts

+    ]);
+  });
+
+  test("THEN `NEXT_PUBLIC_ASSET_PREFIX` control path generatoin", async ({


nitpick: typo

9d8295b (#3580)

namjul mentioned this pull request Sep 23, 2022

fix: render favicon when assetsPrefix is set #3571

Merged

14 tasks

kevinslin reviewed Sep 25, 2022

View reviewed changes

namjul added 6 commits September 26, 2022 13:01

chore: remove unused cypress.json file

dc476e9

chore: rename playwright test folder to e2e

d410529

chore(publish): add NODE_ENV to env handler

0d226aa

chore(publish, test): correct ts types

b27bd19

chore(publish): split env into client and schema

8998df0

chore(publish): test to make sure NEXT_PUBLIC_ASSET_PREFIX env is used

a0baf47

namjul force-pushed the enhance/nextjs-playwright-tests branch from a0fab6d to a0baf47 Compare September 26, 2022 11:24

namjul commented Sep 26, 2022

View reviewed changes

packages/nextjs-template/utils/links.ts Show resolved Hide resolved

chore(publish): correct test description

22f70a7

namjul commented Sep 26, 2022

View reviewed changes

packages/nextjs-template/playwright.config.ts Show resolved Hide resolved

namjul added 5 commits September 26, 2022 15:27

chore(publish): update playwright to use new toHaveScreenshot api

0263076

This is a more ergonomic API and want to use that inside [[dendron://dendron.docs/pkg.nextjs-template.qa.test]].

chore(publish): revert changes

0ca0c5d

Lets keep this how it was for now.

chore(publish): update visual snapshots

4460801

chore(publish): revert logic to allow NODE_ENV=test

5e6517f

chore(publish): consider NEXT_PUBLIC_ASSET_PREFIX when running e2e …

54cf881

…test

namjul force-pushed the enhance/nextjs-playwright-tests branch from b10901d to 54cf881 Compare September 29, 2022 11:55

namjul added 2 commits September 29, 2022 14:38

chore: adjust command to forward arguments intuitively

9d8d958

This commit aims to allow `yarn run ci:test:template:docker -u` or any other multiple arguments appendix.

chore(publish): update e2e VRT snapshots

6e3d250

The cause of the changes is an update from playwright and its browser dependencies.

namjul marked this pull request as ready for review September 29, 2022 12:52

auto-assign bot requested review from Harshita-mindfire and tma66 September 29, 2022 12:52

auto-assign bot assigned kevinslin Sep 29, 2022

auto-assign bot assigned tma66 Sep 29, 2022

namjul added 2 commits September 29, 2022 17:37

chore(publish): add e2e test for NEXT_PUBLIC_ASSET_PREFIX

9e892e5

chore(publish): fix react key warning

a073c04

kevinslin approved these changes Sep 29, 2022

View reviewed changes

chore(publish, e2e): rephrase test case description

9d8295b

namjul merged commit 55c6535 into master Sep 30, 2022

namjul deleted the enhance/nextjs-playwright-tests branch September 30, 2022 11:52

namjul changed the title ~~enhance/nextjs playwright tests~~ chore(publish): improve playwright tests Sep 30, 2022

namjul changed the title ~~chore(publish): improve playwright tests~~ chore(publish): improve e2e tests Sep 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(publish): improve e2e tests #3580

chore(publish): improve e2e tests #3580

namjul commented Sep 23, 2022 •

edited

kevinslin left a comment

namjul commented Sep 26, 2022 •

edited

kevinslin commented Sep 26, 2022

namjul commented Sep 27, 2022 •

edited

kevinslin commented Sep 27, 2022

namjul commented Sep 29, 2022 •

edited

namjul commented Sep 29, 2022 •

edited

kevinslin commented Sep 29, 2022

kevinslin left a comment

kevinslin Sep 29, 2022

namjul Sep 30, 2022

chore(publish): improve e2e tests #3580

chore(publish): improve e2e tests #3580

Conversation

namjul commented Sep 23, 2022 • edited

Pull Request Checklist

kevinslin left a comment

Choose a reason for hiding this comment

namjul commented Sep 26, 2022 • edited

kevinslin commented Sep 26, 2022

namjul commented Sep 27, 2022 • edited

kevinslin commented Sep 27, 2022

namjul commented Sep 29, 2022 • edited

namjul commented Sep 29, 2022 • edited

kevinslin commented Sep 29, 2022

kevinslin left a comment

Choose a reason for hiding this comment

kevinslin Sep 29, 2022

Choose a reason for hiding this comment

namjul Sep 30, 2022

Choose a reason for hiding this comment

namjul commented Sep 23, 2022 •

edited

namjul commented Sep 26, 2022 •

edited

namjul commented Sep 27, 2022 •

edited

namjul commented Sep 29, 2022 •

edited

namjul commented Sep 29, 2022 •

edited