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

Add unit tests for file ignore function in api-server watcher #3342

Open
dac09 opened this issue Sep 3, 2021 · 9 comments
Open

Add unit tests for file ignore function in api-server watcher #3342

dac09 opened this issue Sep 3, 2021 · 9 comments

Comments

@dac09
Copy link
Collaborator

dac09 commented Sep 3, 2021

Related to #3338

In https://github.com/redwoodjs/redwood/blob/main/packages/api-server/src/watch.ts#L61-L66 - the ignored function has lead to some issues on Windows (see linked issue).

It seems odd that chokidar sends a unix-style path even on windows, and just to make sure we don't run into odd situations, we should:

  • move the function to a different file (because this file directly invokes chokidar)
  • add unit tests using the fixtures in the repo, that will let us verify across all platforms, incase things change in the future.
@redwoodjs-bot redwoodjs-bot bot added this to New issues in Current-Release-Sprint Sep 3, 2021
@thedavidprice thedavidprice added this to the next-release-priority milestone Sep 8, 2021
@redwoodjs-bot redwoodjs-bot bot moved this from New issues to In progress (priority) in Current-Release-Sprint Sep 8, 2021
@thedavidprice
Copy link
Contributor

@cannikin would you be available to help @dac09 out with this one for the current release sprint?

@thedavidprice thedavidprice moved this from In progress (priority) to v0.39 in Current-Release-Sprint Nov 1, 2021
@cannikin
Copy link
Member

cannikin commented Nov 2, 2021

This appears to be vaguely related to Windows, remember my caveats ;)

@thedavidprice
Copy link
Contributor

Lulz. But the unit test is, well, just a unit test!

If not, @simoncrypta what do you think about taking a stab at this one?

@thedavidprice thedavidprice assigned simoncrypta and unassigned cannikin Nov 2, 2021
@simoncrypta
Copy link
Collaborator

Let's go ! 👍

@dac09
Copy link
Collaborator Author

dac09 commented Nov 3, 2021

Let me know if you need any help @simoncrypta!

@simoncrypta
Copy link
Collaborator

simoncrypta commented Nov 10, 2021

It seems odd that chokidar sends a unix-style path even on windows

I try to dig into why this odd behaved happens. First thing we should be aware is when we talk about windows, it can be different shell and environment.

  1. Command Prompt
  2. PowerShell
  3. Git Bash
  4. WSL

We can say that Command Prompt and PowerShell are pretty similar for our use case. PowerShell add many powerful features and is much more up to date, for this reason I think we should only support PowerShell in case we need these extra features.

Git Bash, it's package that contains bash with some Unix utilities. It's also use mintty, a terminal emulator for Cygwin. And, Cygwin, is a POSIX-compatible programming and runtime environment that runs natively on Windows. This is maybe why we receive *nix-style path.

Finally, WSL (Windows Subsystem Linux) is more like a virtual machine of Linux with some nice performance and DX optimization. So we should refer WSL as a Linux environment.

So in parallel with #3685, should we make test for all these options, 2 or only one. If we focus only on WSL, we can skip all the thing about windows specific. If we add PowerShell support, we can skip edge cases of using batch on Windows and focus on Windows native environment.

move the function to a different file (because this file directly invokes chokidar)

Just to be sure, we talk about which function? @dac09

@dac09
Copy link
Collaborator Author

dac09 commented Nov 10, 2021

I think I was originally referring to these lines here: https://github.com/redwoodjs/redwood/blob/main/packages/api-server/src/watch.ts#L85-L89

And this ignored function: https://github.com/redwoodjs/redwood/blob/main/packages/api-server/src/watch.ts#L95-L111

Including screenshot incase line numbers change again.
Screenshot 2021-11-10 at 19 12 13

@dac09
Copy link
Collaborator Author

dac09 commented Nov 10, 2021

I'm trying to recall the issue.... if it remember correctly - on windows the dist files were being watched. I fixed this problem here:
https://github.com/redwoodjs/redwood/pull/3341/files

So I believe what I was suggesting in this (poorly written) issue was:
a) Move that "ignored" function so we can write a unit test for it. It probably doesn't need to be a separate file though (maybe something like shouldIgnoreFileForBuild)
b) Add jest tests where we pass in paths from the fixtures folder (__fixtures__/example-todo-main)

@simoncrypta
Copy link
Collaborator

simoncrypta commented Nov 10, 2021

So I believe what I was suggesting in this (poorly written) issue was:

Ah! This is what I understand, but I was about to ask exactly that 😆

Thanks for your help !

@thedavidprice thedavidprice changed the title Add unit tests for file ignore function in api-server wathcer Add unit tests for file ignore function in api-server watcher Nov 10, 2021
@thedavidprice thedavidprice moved this from v0.39 (locked) to In progress (priority) in Current-Release-Sprint Nov 10, 2021
@jtoar jtoar removed this from the next-release-priority milestone Nov 17, 2021
@redwoodjs-bot redwoodjs-bot bot removed this from In progress (priority) in Current-Release-Sprint Nov 17, 2021
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 a pull request may close this issue.

5 participants