Windows CI Failing Tests: fix Jest test issues when run on Windows

[thedavidprice]

Overview

Redwood's CI has 40 failing tests on the Windows runner. This is an issue with CLI rw generate command's related tests and fixtures. And in most cases, the issue is due to a lack of Windows' path support.

related PR #543

Current Status and Plan

The Windows CI will continue to run (and fail). However, only the Ubuntu CI runner is required to pass in order to merge a PR.

Edit: Windows has been removed from CI in master. PR to re-add here #543

We would greatly appreciate any+all community help to resolve these issues and get all tests to pass on Windows! 🙏🚀

NOTE: to contribute fixed tests, please create a PR against branch: dsp-fix-Windows-CI-runner-support`

Details

For example, see the "Run tests" section of this GH Action run:
https://github.com/redwoodjs/redwood/pull/527/checks?check_run_id=661166735#step:8:1

The failing tests are all within the CLI package and related to generate.js tests, which are most often failing when testing paths. For Example (link to output):

@redwoodjs/cli:     Expected: "\\path\\to\\project\\api\\src\\functions\\func.js"
@redwoodjs/cli:     Received: "/path/to/project/api/src/functions/func.js"

Prior to incorrectly assuming we had resolved these issues (100% on me 😬), Peter attempted to add support for Windows by normalizing the paths. You can view his initial work here. However, this normalization does not yet work in all cases where it was applied, nor was normalization applied to all tests.

• need to validate the correct normalization approach(es) to the different types of test failures
• PR contributions can be as granular as fixing tests per file, e.g. fixing helpers.test.js or service.test.js -- there's no need to fix everything at once

[thedavidprice]

@chrissutton Here's the overview Issue I mentioned. First off, please don't be overwhelmed with all the details or quantity of failing tests -- no expectations here for a full deep-dive. Here's the high level:

• we have a CLI command for code generation (inspired by Rails)
  • the CLI is NodeJS using Yargs library (not super important)
• to test individual generators, we've created a lot of fixtures (mock code templates, effectively) and file path checks
• BUT, we're running into "false negatives" when we run these tests in a Windows environment — our mock paths (i.e. hardcoded paths) are not cross-platform compatible.

If you have experience here, we're looking for direction in the form of some "how-to" steps to fix this. We'll do the legwork if someone can point the way. But, lastly, I know Node hasn't been your focus right now, so 100% understood if this is currently a bit out of your wheelhouse. All good.

[thedavidprice]

@kimadeline I just saw from your profile you're at Microsoft (true?). If so, and your dev system is Windows, heads up that you might be running into this issue with local tests as well as the CI runner. We broke a lot of things on Windows 'cause, it turns out, we're bad at it.

I would welcome any thoughts/direction/feedback from you about this if you have a chance. No pressure either way.

Last note: Chris (comment above) is a good friend of mine and also at Microsoft. So no worries about stepping on toes one way or the other. All good in the name of collaboration.

[kimadeline]

Thanks for the heads-up @thedavidprice! I do indeed work at Microsoft, but my primary dev machine is a Mac 😬

I saw that path.normalize was used in #543, is it because we are making (possibly incorrect) assumptions on how it's dealing with separators, kinda like in this node issue? Should we use something else instead? I usually build paths with path.join, but I understand that it may quickly get tedious/out of hand/not best practice.

Yep I used we, we're in this CI-failure boat together ⛵

[thedavidprice]

Yep I used we, we're in this CI-failure boat together ⛵

^^ 😆Welcome aboard! It's a little choppy at times, but there are lots of great people to make it worth the ride. (Which includes friendly banter. What we lack in TS and Window's skills, we make up for in banter... pros/cons.)

re: path.normalize
That was effectively a quick experiment to see if we could get some of the tests passing. So nothing more than the last attempt and where we left off. And most likely a) incorrect and b) in need of using something else.

re: path.join
We use a whole lot of path.join around here. You'll likely see this helper, from paths.ts, being used frequently to handle 'cwd' issues related to Yarn Workspaces.

And as an adjacent issue re: Windows + Paths, in some of our commands we might be potentially overcorrecting. For example, see this line in the dev.js command (i.e. yarn rw dev). We originally had an issue for Windows users reporting an error when their path included a space. (Which, now looking back, was most likely due to the Prisma Client not supporting spaces in paths.) So we fixed it with that hack, but now it's affecting both Windows and Mac users. See this conversation in progress.

¯_(ツ)_/¯

---

That said, please know I'm providing all this primarily for context -- not expectations. Any/all help on the Windows/CI/paths front would be very helpful. But the work on the generator TS conversation/support is definitely a priority.

[Tobbe]

Just leaving some notes here after taking a look at this issue. Maybe it will help someone else getting up to speed on this issue faster. And/or spark some ideas with someone 🙂

1. To get the tests running at all, go to the root of this repo and run yarn install. You can ignore missing peer deps.
2. You can now run yarn test and all tests, except the CLI tests, should pass. You will be absolutely flooded by errors from the CLI tests
3. To be less overwhelmed you can do cd packages/cli and from there execute a single test file by running node ../../node_modules/jest/bin/jest.js -i src/commands/generate/page/__tests__/page.test.js
4. Currently the file above has 8 tests, 4 which are passing and 4 which are failing.

   $ node ../../node_modules/jest/bin/jest.js -i src/commands/generate/page/__tests__/page.test.js
    FAIL  src/commands/generate/page/__tests__/page.test.js (5.467s)
     √ returns exactly 2 files (4ms)
     × creates a page component (4ms)
     × creates a page test
     × creates a page component (1ms)
     × creates a page test (2ms)
     √ creates a single-word route name (1ms)
     √ creates a camelCase route name for multiple word names (1ms)
     √ creates a path equal to passed path
   

   To run just one of the failing tests, open the test file and change test('creates a pa... to test.only('creates a pa...
5. You can now run the test file again, with node ../../node_modules/jest/bin/jest.js -i src/commands/generate/page/__tests__/page.test.js and only the selected test case will execute.

Following the steps above I could finally start digging in to what was actually causing the test to fail.

The problem seems to be that templateForComponentFile returns a path that looks like \\path\\to\\project\\web\\src\\pages\\HomePage\\HomePage.js, which is then used as a key in singleWordFiles in the testcase. When the test case tried to do a lookup on that object it uses /path/to/project/web/src/pages/HomePage/HomePage.js as the key, which doesn't exist, thus getting undefined as the "expected" value for the test.

[Tobbe]

Fixing the key lookup was as simple as wrapping the key in path.normalize.

 test('creates a page component', () => {
   expect(
-    singleWordFiles['/path/to/project/web/src/pages/HomePage/HomePage.js']
+    singleWordFiles[
+      path.normalize('/path/to/project/web/src/pages/HomePage/HomePage.js')
+    ]
   ).toEqual(loadGeneratorFixture('page', 'singleWordPage.js'))
 })

This works because Windows actually understands both \ and / path separators, but prefers \ and so will convert the / in the string to \
On Posix I hope path.normalize will not modify the string at all

Fixing that got me one step further. Next error was this:

  ● creates a page component

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

    @@ -1,10 +1,10 @@
      const HomePage = () => {
        return (
          <div>
            <h1>HomePage</h1>
    -       <p>Find me in ./web/src/pages/HomePage/HomePage.js</p>
    +       <p>Find me in ./web\src\pages\HomePage\HomePage.js</p>
          </div>
        )
      }

And this is were things get ugly 🙁

My "fix" for that is:

diff --git a/packages/cli/src/commands/generate/helpers.js b/packages/cli/src/commands/generate/helpers.js 
index 388ade1..5340653 100644                                                                              
--- a/packages/cli/src/commands/generate/helpers.js                                                        
+++ b/packages/cli/src/commands/generate/helpers.js                                                        
@@ -39,7 +39,10 @@ export const templateForComponentFile = ({                                              
     path.join(generator, 'templates', templatePath),                                                      
     {                                                                                                     
       name,                                                                                               
-      outputPath: `./${path.relative(getPaths().base, componentOutputPath)}`,                             
+      outputPath: `.${path.sep}${path.relative(                                                           
+        getPaths().base,                                                                                  
+        componentOutputPath                                                                               
+      )}`.replace(/\\/g, '/'),                                                                            
       ...templateVars,                                                                                    
     }                                                                                                     
   )                                                                                                       

With that hack in place all eight tests in src/commands/generate/page/tests/page.test.js passes.

Any ideas how to properly fix the "Find me" path in the generated file? Or is this good enough (for now)? Could this break anything that's currently working?

[thedavidprice]

@Tobbe hack or no hack, this is fantastic. And to your question "could this break things currently working?" --> I created a specific branch for this very purpose.

Could you open a PR against branch dsp-fix-Windows-CI-runner-support? Let's see what happens!

[Tobbe]

Will do, but I spent way over my allotted play hours on this today already 😛 Probably spent tomorrow's hours as well 😄

[thedavidprice]

Well, I can't say enough how valuable this help is. Huge thank you!

I'll keep fanning the flame here, but also loop me in for implementation help+needs as you go.

[Tobbe]

Initial PR for just a few tests #656