Skip to content

Fix RWC missing file detection#46673

Merged
weswigham merged 1 commit intomicrosoft:mainfrom
weswigham:fix-rwc
Nov 9, 2021
Merged

Fix RWC missing file detection#46673
weswigham merged 1 commit intomicrosoft:mainfrom
weswigham:fix-rwc

Conversation

@weswigham
Copy link
Copy Markdown
Member

@weswigham weswigham commented Nov 3, 2021

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.

@weswigham weswigham requested review from amcasey and sandersn November 3, 2021 20:55
@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
Copy Markdown
Member

andrewbranch commented Nov 3, 2021

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
Copy Markdown
Member Author

weswigham commented Nov 3, 2021

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
Copy Markdown
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
Copy Markdown
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
@andrewbranch
Revert "Fix RWC missing file detection (microsoft#46673)"
931b504 
This reverts commit 4a065f5.
This was referenced Nov 11, 2021
Open
Merged
typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Nov 11, 2021
@andrewbranch @typescript-bot
Cherry-pick PR microsoft#46787 into release-4.5
8cc2c5f 
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
@andrewbranch
Revert changes to matchFiles/readDirectory made since 4.3 (#46787)
1298f49 
* 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
@andrewbranch
Cherry-pick PR #46787 into release-4.5 (#46789)
460515f 
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 <andrew@wheream.io>
@rbuckton
Copy link
Copy Markdown
Contributor

rbuckton commented Nov 13, 2021

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
@weswigham @mprobst
Fix RWC missing file detection (microsoft#46673)
ae5d389
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
@andrewbranch @mprobst
Revert changes to matchFiles/readDirectory made since 4.3 (microsoft#…
bdd324e 
…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
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sandersn sandersn sandersn approved these changes

@amcasey amcasey Awaiting requested review from amcasey

Assignees

@weswigham weswigham

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@weswigham @andrewbranch @amcasey @rbuckton @sandersn @typescript-bot