test(v2): fix Jest tests on Windows #3540

skrmain · 2020-10-05T16:52:48Z

Motivation

I want to fix the Test Cases for Windows OS.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

I have run test on

Windows Locally - yarn test, yarn start, yarn start:v1, yarn prettier && yarn lint
Linux on GitPod - yarn test, yarn start, yarn start:v1, yarn prettier && yarn lint

Related PRs

#3481

TODOs - packages to be fixed

docusaurus-plugin-client-redirects
docusaurus-plugin-content-docs
docusaurus-plugin-content-blog
docusaurus-plugin-content-pages
docusaurus-mdx-loader
docusaurus-utils
docusaurus-1.x
Add window test configuration in nodejs-windows.yml

- add the posixPath in writeRedirectsFiles.ts

docusaurus-bot · 2020-10-05T16:59:28Z

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit e311d53

https://deploy-preview-3540--docusaurus-2.netlify.app

lex111 · 2020-10-05T17:08:41Z

@slorber I think it made sense to merge PR #3481 to see successful progress in this PR (CI will not fails).

slorber · 2020-10-05T17:13:08Z

@lex111 I don't think it's a good idea because this would lead to master failing on windows CI and all created PRs after the merge would also fail on windows and let contributors think they did something wrong. Also we are not sure yet how easy it is to fix all the windows tests

slorber · 2020-10-05T17:14:20Z

However we should be able to easily track progress in this PR.

maybe @skr571999 you can enable windows tests here so that we'll see this PR failing with the number of failing tests decreasing as your work progress?

skrmain · 2020-10-05T17:18:01Z

Yeah ok, @slorber

In the next commit I will add that.

lex111 · 2020-10-05T17:18:57Z

@slorber agree, but it's better to close PR #3481, there's no point in that.

add posixPath in test and index

- add the window test configuration in nodejs-windows.yml

skrmain · 2020-10-06T14:26:48Z

Hi, @slorber

Any idea why it is it failing the yarn build:v2

On my local system it is building fine

slorber · 2020-10-07T13:37:37Z

Hi @skr571999

No idea sorry 😅 maybe try to revert some of the changes until it passes?

slorber · 2020-10-07T13:38:54Z

packages/docusaurus-plugin-content-pages/src/index.ts

@@ -96,7 +97,7 @@ export default function pluginContentPages(

      function toMetadata(relativeSource: string): Metadata {
        const source = path.join(pagesDir, relativeSource);
-        const aliasedSourcePath = aliasedSitePath(source, siteDir);
+        const aliasedSourcePath = posixPath(aliasedSitePath(source, siteDir));


I guess it's why CI build fails, because it's the only non-test code change related to pages?

skrmain · 2020-10-09T13:11:51Z

Hi @slorber @lex111

Getting mdx-loader/transformImage Test case fail on GitHub Action due to extra ../ in the Received Path

But it is running fine on my machine

My Machine

Window 10 Home - Version(2004)

Not getting How to check on my local system that it is fine or not.

Any idea how i can test on my system locally getting the same response as on Github Actions

slorber · 2020-10-12T10:58:28Z

Thanks for reporting @skr571999 , don't know why it only works on your computer but not CI.

Looks to me there are quite a few snapshot errors due to snapshots here: https://github.com/facebook/docusaurus/pull/3540/checks?check_run_id=1229879405

As you modify both the tests and the implementation at the same time, it's hard to tell if it can't lead to regressions.
Can you comment on your PRs about why you choose to modify each line to help me understand? Particularly the "cleanPath", that looks surprising to me that we need this line

skrmain · 2020-10-12T15:48:07Z

packages/docusaurus-mdx-loader/src/remark/transformImage/index.js

+ * Revove replace the starting "../" with "".
+ * E.g: ../package/doc -> package/doc
+ */
+function cleanPath(filePath) {


Before adding the cleanPath Test Case Result

the above test cases are failing because of the extra ../

After Adding the cleanPath Test Case Result

so, basically cleanPath is just to remove the extra ../ from the path if it exists

Also one more thing all the 4 Test case are passing Completely fine in My Local system even without cleanPath.

But why are the tests failing due to the extra ../ in the first place? The goal is not only to make the tests pass, also to understand why the solution solves the problem. Where does this extra ../ come from and why only on Windows?

But why are the tests failing due to the extra ../ in the first place? The goal is not only to make the tests pass, also to understand why the solution solves the problem.

Yeah right, I will look and update.

Where does this extra ../ come from and why only on Windows?

the extra ../ is not coming in my local System it is here only 😔.

Hi @slorber

I tried finding the reason why extra ../ is coming.
so it did logging the process.cwd() then i found the response different in my local system and different in Github CI testing

On my local system

On GitHub

due to this difference in path when we are finding the relative path in our code it is adding the extra ../

oh, this is weird, thanks for letting me know

skrmain · 2020-10-12T15:49:02Z

Hi @slorber

I have added the comment for review(contain the description about cleanPath function),

As you modify both the tests and the implementation at the same time, it's hard to tell if it can't lead to regressions.

I have revert back the old commit, because that was also having something similar problem, so i thought to try this one and look if it is happening here or not.

Can u have a look and suggest what I can do because I am not able to test the changes Locally as they are passed locally and are failing here 😂

😅 I am not sure but as in GitHub Action we are having Windows Server OS and in my local system I am having Window 10 Home, is this can be the cause of the different Output of the Test Cases??

slorber · 2020-10-14T10:52:23Z

Hi,

As I don't even have windows right now it's hard for me to review this PR.
I don't now either why it works locally and fails on the CI and what to do about it.

Maybe we should put this PR in stand-by until I have Windows and can provide more help?

slorber · 2020-12-28T11:47:32Z

closed in favor of #3959

the new pr is based on your initial commits so you'll be credited for your work

slorber · 2020-12-28T18:50:34Z

fixed in #3959

skrmain added 2 commits October 5, 2020 22:04

test(v2): Fix docusaurus-utils tests for windows

1ddcef9

test(v2): Fix plugin-client-redirects test

c08a93e

- add the posixPath in writeRedirectsFiles.ts

skrmain requested review from lex111 and slorber as code owners October 5, 2020 16:52

lex111 changed the title ~~test(v2) : Fix window tests~~ test(v2): fix Jest tests on Windows Oct 5, 2020

slorber mentioned this pull request Oct 5, 2020

chore(v2): run Jest tests on Windows workflow #3481

Closed

facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 5, 2020

skrmain added 3 commits October 6, 2020 18:13

test(v2): Fix plugin-content-pages test

eba618e

add posixPath in test and index

Merge branch 'master' into fix-window-tests

e04382d

test(v2): add window test configuration

bda3adb

- add the window test configuration in nodejs-windows.yml

slorber reviewed Oct 7, 2020

View reviewed changes

skrmain added 4 commits October 8, 2020 16:28

test(v2): revert plugin-content-pages test fix

c530f0e

Merge branch 'master' into fix-window-tests

916dae7

test(v2): Fix mdx-loader/transformImage test

5541b48

test(v2): add cleanPath in transformImage test

e311d53

skrmain commented Oct 12, 2020

View reviewed changes

slorber marked this pull request as draft October 19, 2020 16:12

slorber mentioned this pull request Dec 28, 2020

fix(v2): fix redirect toUrl (windows + trailing slash) #3903

Merged

slorber mentioned this pull request Dec 28, 2020

chore(v2): fix windows Jest tests #3959

Merged

slorber closed this Dec 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(v2): fix Jest tests on Windows #3540

test(v2): fix Jest tests on Windows #3540

skrmain commented Oct 5, 2020 •

edited

Loading

docusaurus-bot commented Oct 5, 2020 •

edited

Loading

lex111 commented Oct 5, 2020

slorber commented Oct 5, 2020

slorber commented Oct 5, 2020

skrmain commented Oct 5, 2020

lex111 commented Oct 5, 2020

skrmain commented Oct 6, 2020 •

edited

Loading

slorber commented Oct 7, 2020

slorber Oct 7, 2020

skrmain commented Oct 9, 2020 •

edited

Loading

slorber commented Oct 12, 2020

skrmain Oct 12, 2020

slorber Oct 12, 2020

skrmain Oct 13, 2020

skrmain Oct 15, 2020

slorber Oct 16, 2020

skrmain commented Oct 12, 2020

slorber commented Oct 14, 2020

slorber commented Dec 28, 2020

slorber commented Dec 28, 2020

test(v2): fix Jest tests on Windows #3540

test(v2): fix Jest tests on Windows #3540

Conversation

skrmain commented Oct 5, 2020 • edited Loading

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

TODOs - packages to be fixed

docusaurus-bot commented Oct 5, 2020 • edited Loading

lex111 commented Oct 5, 2020

slorber commented Oct 5, 2020

slorber commented Oct 5, 2020

skrmain commented Oct 5, 2020

lex111 commented Oct 5, 2020

skrmain commented Oct 6, 2020 • edited Loading

slorber commented Oct 7, 2020

slorber Oct 7, 2020

Choose a reason for hiding this comment

skrmain commented Oct 9, 2020 • edited Loading

slorber commented Oct 12, 2020

skrmain Oct 12, 2020

Choose a reason for hiding this comment

Before adding the cleanPath Test Case Result

After Adding the cleanPath Test Case Result

slorber Oct 12, 2020

Choose a reason for hiding this comment

skrmain Oct 13, 2020

Choose a reason for hiding this comment

skrmain Oct 15, 2020

Choose a reason for hiding this comment

slorber Oct 16, 2020

Choose a reason for hiding this comment

skrmain commented Oct 12, 2020

slorber commented Oct 14, 2020

slorber commented Dec 28, 2020

slorber commented Dec 28, 2020

skrmain commented Oct 5, 2020 •

edited

Loading

docusaurus-bot commented Oct 5, 2020 •

edited

Loading

skrmain commented Oct 6, 2020 •

edited

Loading

skrmain commented Oct 9, 2020 •

edited

Loading

Before adding the `cleanPath` Test Case Result

After Adding the `cleanPath` Test Case Result