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

fix: check find binary supports the -iname parameter #10308

Merged
merged 4 commits into from
Jul 24, 2020

Conversation

richardlau
Copy link
Contributor

Summary

The -iname parameter is a non-POSIX extension and may not be supported
by a POSIX compliant find binary. Add a check that the parameter is
supported when determining whether to use the native find binary.

Fixes: #10263

Test plan

Wrote included test first that failed. Test passes with changes.

@richardlau
Copy link
Contributor Author

This is the first PR I've made to a Facebook open source project. I have signed the CLA.

@richardlau richardlau force-pushed the find branch 2 times, most recently from 7505a40 to ba354fe Compare July 23, 2020 19:43
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

packages/jest-haste-map/src/crawlers/node.ts Outdated Show resolved Hide resolved
@richardlau
Copy link
Contributor Author

Thanks! Can we remove the windows check now?

I think so but when I removed https://github.com/facebook/jest/blob/7eff69bf4e7e1f59036af0821056dc0c36967725/packages/jest-haste-map/src/crawlers/__tests__/node.test.js#L138

and ran the test suite on Windows I started getting test failures like:

  ● node crawler › uses node fs APIs if "forceNodeFilesystemAPI" is set to true, regardless of platform

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

    - Expected  - 9
    + Received  + 1

    @@ -1,15 +1,7 @@
      Map {
    -   "fruits/directory/strawberry.js" => Array [
    -     "",
    -     33,
    -     42,
    -     0,
    -     "",
    -     null,
    -   ],
    -   "fruits/tomato.js" => Array [
    +   "fruits\\tomato.js" => Array [
          "",
          32,
          42,
          0,
          "",

      at toEqual (packages/jest-haste-map/src/crawlers/__tests__/node.test.js:362:30)

so I was a bit hesitant to remove that check. Now I have some working code (the PR as it currently is) I'll take a look again (this is my first time with Jest and TypeScript so I've had to learn a lot to get to this point).

@SimenB
Copy link
Member

SimenB commented Jul 24, 2020

so I was a bit hesitant to remove that check

Seems we should normalize the separator in the paths a bit. We've used slash a few places for this, maybe worth a go? I don't have access to a Windows machine myself unfortunately, so I'm not able to provide much help 😞

this is my first time with Jest and TypeScript so I've had to learn a lot to get to this point

Thank you so much for spending the time! 🙏

@SimenB
Copy link
Member

SimenB commented Jul 24, 2020

Btw, making those unit tests work on Windows is not in any way a blocker. We run almost all integration tests on windows as well, and if removing that check for windows still passes CI I'm happy to merge while still skipping the unit tests on Windows 🙂

@richardlau
Copy link
Contributor Author

I have the jest-haste-map test suite passing on Windows locally now (🎉). Hopefully the CI agrees 🤞.

The `-iname` parameter is a non-POSIX extension and may not be supported
by a POSIX compliant find binary. Add a check that the parameter is
supported when determining whether to use the native find binary.
Enable detection of a compatible find binary on Windows. Fix tests
so that they run and pass on Windows.
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thank you!

@SimenB SimenB changed the title Check find binary supports the -iname parameter fix: check find binary supports the -iname parameter Jul 24, 2020
@SimenB SimenB merged commit 6ad3a74 into jestjs:master Jul 24, 2020
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
@richardlau richardlau deleted the find branch May 11, 2021 05:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native file crawling doesn't work with POSIX find
3 participants