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 RWC missing file detection #46673

Merged
merged 1 commit into from
Nov 9, 2021
Merged

Fix RWC missing file detection #46673

merged 1 commit into from
Nov 9, 2021

Conversation

weswigham
Copy link
Member

RWC has been failing for awhile (2 weeks); it wasn't really noticeable, since we still got normal diff PRs and merged them as needed, which seemed fine, but today I noticed that it was still failing even immediately after I merged an update PR - finding that suspect, I checked the logs and found out that pretty much every test was giving a spurious error saying it was missing every file in the test. Definitely not right. The story ended up being that #46086 made our readDirectory return absolute paths, while the rwc harness was very much relying on relative paths to perform a simple path comparison.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 3, 2021
@andrewbranch
Copy link
Member

Hm, do we have an opinion on what the correct thing for readDirectory to return is? There seemed to be some internal inconsistency that led to a bug, but I didn't fully realize that fixing the bug also resulted in such an observable API change.

@weswigham
Copy link
Member Author

Hm, do we have an opinion on what the correct thing for readDirectory to return is?

I'd defer to @rbuckton personally, since he probably knows what the intent was, since it was written to support file globbing, iirc.

@amcasey
Copy link
Member

amcasey commented Nov 3, 2021

For the record, I'm totally cool with rolling back the original change (#44710) - ideally just this part, but the whole thing if that's easier. It was a minor quality-of-life improvement (can enable exceptions during debugging with minimal noise).

@amcasey
Copy link
Member

amcasey commented Nov 3, 2021

Also, I'd expect to get names and not paths from readDirectory - I'm pretty sure that's what fs.readdir returns.

@weswigham weswigham merged commit 4a065f5 into microsoft:main Nov 9, 2021
@weswigham weswigham deleted the fix-rwc branch November 9, 2021 18:41
andrewbranch added a commit to andrewbranch/TypeScript that referenced this pull request Nov 11, 2021
typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Nov 11, 2021
Component commits:
931b504 Revert "Fix RWC missing file detection (microsoft#46673)"
This reverts commit 4a065f5.

afef282 Revert "Pass absolute path to directoryExists (microsoft#46086)"
This reverts commit 55b4928.

f1a20b3 Revert "Reduce exceptions (microsoft#44710)"
This reverts commit c0d5c29.

56842cd Add back system watcher limit
andrewbranch added a commit that referenced this pull request Nov 11, 2021
* Revert "Fix RWC missing file detection (#46673)"

This reverts commit 4a065f5.

* Revert "Pass absolute path to directoryExists (#46086)"

This reverts commit 55b4928.

* Revert "Reduce exceptions (#44710)"

This reverts commit c0d5c29.

* Add back system watcher limit
andrewbranch added a commit that referenced this pull request Nov 11, 2021
Component commits:
931b504 Revert "Fix RWC missing file detection (#46673)"
This reverts commit 4a065f5.

afef282 Revert "Pass absolute path to directoryExists (#46086)"
This reverts commit 55b4928.

f1a20b3 Revert "Reduce exceptions (#44710)"
This reverts commit c0d5c29.

56842cd Add back system watcher limit

Co-authored-by: Andrew Branch <[email protected]>
@rbuckton
Copy link
Member

Also, I'd expect to get names and not paths from readDirectory - I'm pretty sure that's what fs.readdir returns.

I mentioned this in #46788, but readDirectory wasn't intended to emulate fs.readdir. When globs were added, one of the requirements was that readDirectory return all of the matched results in one API call. This was because VSTS wanted to be able to swap out a call to readDirectory with one performed in-process to avoid the overhead of multiple round-trips from .NET to JavaScript using recursive calls that only returned file names.

mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
…46787)

* Revert "Fix RWC missing file detection (microsoft#46673)"

This reverts commit 4a065f5.

* Revert "Pass absolute path to directoryExists (microsoft#46086)"

This reverts commit 55b4928.

* Revert "Reduce exceptions (microsoft#44710)"

This reverts commit c0d5c29.

* Add back system watcher limit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants