Skip to content

Fix test case recorder #1341

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

Merged
merged 2 commits into from
Mar 23, 2023
Merged

Fix test case recorder #1341

merged 2 commits into from
Mar 23, 2023

Conversation

pokey
Copy link
Member

@pokey pokey commented Mar 21, 2023

The test case recorder broke in #1281 due to the fact we're now launching extension in subdirectory called dist rather than top-level repo. This PR fixes it

Checklist

@pokey pokey force-pushed the pokey/fix-test-case-recorder branch from 62dbc11 to 5d05b67 Compare March 21, 2023 15:23
@pokey pokey requested a review from AndreasArvidsson March 21, 2023 15:23
@pokey pokey enabled auto-merge March 21, 2023 15:30
@pokey pokey disabled auto-merge March 21, 2023 15:57
@pokey pokey marked this pull request as draft March 21, 2023 16:06
@pokey pokey marked this pull request as ready for review March 21, 2023 19:02
@pokey pokey enabled auto-merge March 21, 2023 19:02
@pokey
Copy link
Member Author

pokey commented Mar 21, 2023

For some reason that I can't fathom, we're getting a CI failure on Windows because even though the test case recorder is correctly outputting a file with LF endings there, the expected content has CRLF endings. This is quite strange, because we're just reading a gold file that has LF endings. It's almost like GitHub action runner is turning on git config core.autocrlf or something 🤔 cc/ @AndreasArvidsson @auscompgeek any ideas?

I could hack it by running .replace('\r\n', '\n') on the expected content; not a big deal. Strange tho

@auscompgeek
Copy link
Member

It's almost like GitHub action runner is turning on git config core.autocrlf or something

It's probably fairly likely this is enabled in the global config… maybe we should check its value before checkout?

@pokey
Copy link
Member Author

pokey commented Mar 21, 2023

It's probably fairly likely this is enabled in the global config

but how / why? I don't see anywhere that this is something GitHub does in their runners 🤔

@auscompgeek
Copy link
Member

Maybe we should try setting text=lf in .gitattributes 🤔

@pokey
Copy link
Member Author

pokey commented Mar 21, 2023

I'm hesitant to mess with that setting, as I don't fully understand it. It would be great to get this PR in, as the test case recorder is currently broken on main. Shall I file a follow-up to look into line endings?

@Leeft
Copy link

Leeft commented Mar 22, 2023

Maybe we should try setting text=lf in .gitattributes 🤔

Yeah, that seems like a good idea: https://dev.to/deadlybyte/please-add-gitattributes-to-your-git-repository-1jld

@pokey pokey force-pushed the pokey/fix-test-case-recorder branch 2 times, most recently from d9e23b3 to b0727f2 Compare March 22, 2023 14:49
@pokey pokey mentioned this pull request Mar 22, 2023
4 tasks
@pokey pokey changed the base branch from main to pokey/force-line-endings-to-be-lf March 22, 2023 14:58
@pokey pokey disabled auto-merge March 22, 2023 14:59
@pokey
Copy link
Member Author

pokey commented Mar 22, 2023

Ok I filed #1348 to handle line endings properly

@pokey pokey marked this pull request as draft March 22, 2023 15:10
@pokey pokey force-pushed the pokey/fix-test-case-recorder branch 2 times, most recently from a7aab72 to 890c41c Compare March 22, 2023 15:57
@pokey pokey force-pushed the pokey/force-line-endings-to-be-lf branch from 54b954c to 51c2193 Compare March 22, 2023 15:57
@pokey pokey linked an issue Mar 22, 2023 that may be closed by this pull request
@pokey pokey force-pushed the pokey/fix-test-case-recorder branch from 890c41c to 7f09b35 Compare March 22, 2023 20:47
@pokey pokey force-pushed the pokey/fix-test-case-recorder branch 2 times, most recently from b14ed17 to 8c63db1 Compare March 23, 2023 15:05
@pokey pokey changed the base branch from pokey/force-line-endings-to-be-lf to main March 23, 2023 15:06
@pokey pokey force-pushed the pokey/fix-test-case-recorder branch from 8c63db1 to e9f164d Compare March 23, 2023 15:09
@pokey pokey marked this pull request as ready for review March 23, 2023 15:10
@pokey
Copy link
Member Author

pokey commented Mar 23, 2023

Ok @AndreasArvidsson this one should be good to go. I added one more test that runs the test case recorder in the way we commonly do, where the user is prompted for a directory, rather than passing directory in via an argument to record test command. Prior to that, this PR actually had a bit of sleight of hand where I wasn't actually running the test case recorder in the usual way, and in fact it wouldn't have caught the bug that this PR fixes 😅.

Added new ability along the way to fake user input in quick pick; was trivial using our FakeIDE infra

@pokey pokey enabled auto-merge March 23, 2023 15:13
@pokey pokey mentioned this pull request Mar 23, 2023
3 tasks
@pokey pokey added this pull request to the merge queue Mar 23, 2023
Merged via the queue into main with commit 235b1b9 Mar 23, 2023
@pokey pokey deleted the pokey/fix-test-case-recorder branch March 23, 2023 15:48
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 this pull request may close these issues.

Cursorless record is borked for me
4 participants