Skip to content

Conversation

andrewbranch
Copy link
Member

Related to #54556 and #54506. Checked the diff patch with a regex to confirm that changes are either 1:1 path changes or removed lines from module resolution since there are fewer ancestor directories to look for node_modules in. One test file changed to use absolute paths with // @out: since out is super broken.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 7, 2023
@andrewbranch andrewbranch requested review from rbuckton, weswigham and jakebailey and removed request for rbuckton June 7, 2023 16:33
@jakebailey
Copy link
Member

Viewing the diff without the baseline changes, it seems good; we will probably need to be extra vigilant in the next few weeks to ensure that we rerun CI on any outstanding PRs lest they change.

Just to clarify, what is the motivation to remove the prefix? Just that it's weird that we include it? (Which, I agree with, but, confirming).

@andrewbranch
Copy link
Member Author

andrewbranch commented Jun 7, 2023

Before #54556, the prefix was applied inconsistently (within a single test file); e.g. it was prepended to filenames, but not file paths in compiler options like // @outDir: out. Consequently, if you were writing a test that required on a relationship between input and output files, you had to know to include a magical prefix on // @outDir: tests/cases/compiler/out, but not on the // @Filename comments. I believe #54556 fixed this, but it still felt very fragile to me. In #54556, some baselines added the tests/cases/compiler prefix to certain reported filenames, but this PR removes them from many more, which seems to imply that the prefixing logic has always been applied inconsistently (across test files / baselines), in a way that I still don’t understand after spending a miserable few days in the harness code.

@rbuckton
Copy link
Contributor

rbuckton commented Jun 8, 2023

I think we should have the path prefix in the test output in some way, since it helps you find the actual test when comparing diffs since our compiler and performance baselines are just dumped to a single folder. If we're going to make a sweeping change to our test outputs anyways, perhaps we should add a header line to the baseline to indicate the test source location relative to the repo.

@andrewbranch
Copy link
Member Author

I can add that. Maybe there’s a VS Code extension that would recognize file paths in plain text / unrecognized language modes and let you ⌘-click to go to them. That would be nice.

@andrewbranch
Copy link
Member Author

Maybe there’s a VS Code extension that would recognize file paths in plain text / unrecognized language modes and let you ⌘-click to go to them

I added the header, but I’ve yet to find an extension like this.

@andrewbranch
Copy link
Member Author

No baselines have changed in main since opening this. I’m going to go ahead and merge.

@andrewbranch andrewbranch merged commit 2f4962a into main Jun 9, 2023
@andrewbranch andrewbranch deleted the remove-test-rootdir branch June 9, 2023 16:23
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.

5 participants