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

ci: test-readme enhancements #58

Merged
merged 8 commits into from
May 8, 2023
Merged

Conversation

pskfyi
Copy link
Owner

@pskfyi pskfyi commented May 6, 2023

See #47, #59.

@pskfyi pskfyi force-pushed the test-readme-cross-platform branch 2 times, most recently from a48973b to 4336d8e Compare May 6, 2023 20:52
@pskfyi pskfyi changed the title ci: run test-readme in all OS's evalCodeBlocks enhancements May 6, 2023
@pskfyi pskfyi force-pushed the test-readme-cross-platform branch from 4336d8e to 7f79123 Compare May 6, 2023 20:56
@pskfyi pskfyi changed the title evalCodeBlocks enhancements ci: test-readme enhancements May 6, 2023
@pskfyi pskfyi force-pushed the test-readme-cross-platform branch from 971287f to 2948682 Compare May 6, 2023 22:02
@pskfyi pskfyi force-pushed the test-readme-cross-platform branch from 2948682 to db827e1 Compare May 7, 2023 02:12
@pskfyi pskfyi marked this pull request as ready for review May 7, 2023 02:34
@pskfyi pskfyi requested a review from AustinArey May 7, 2023 02:35
@pskfyi pskfyi force-pushed the test-readme-cross-platform branch 2 times, most recently from 76f9f8f to fcdcab7 Compare May 8, 2023 02:25
@pskfyi pskfyi force-pushed the test-readme-cross-platform branch 2 times, most recently from 0f4e200 to 85fdcf4 Compare May 8, 2023 02:37
@pskfyi
Copy link
Owner Author

pskfyi commented May 8, 2023

After rebasing on top of main CI is accurately surfacing a unit test added by this PR which was passing within the PR, which was branched off of an old main, but which fails in the context of the updated main. This is the exact reason that we want to keep PRs in sync with the base branch. I disabled this setting before because the GitHub UI unfortunately defaults to a merge commit rather than rebasing for this. I think we should try enabling for a while and seeing if it causes more issues than it catches.

Fix for the failing test is incoming.

@pskfyi pskfyi force-pushed the test-readme-cross-platform branch 2 times, most recently from 3f61b61 to 6be64e9 Compare May 8, 2023 03:11
@AustinArey AustinArey force-pushed the test-readme-cross-platform branch from 6be64e9 to 4199572 Compare May 8, 2023 03:25
@AustinArey
Copy link
Collaborator

Updated branch via rebase. I see the force push above. Now evalCodeBlocks() test fails.

I did git fetch then git checkout test-readme-cross-platform then git pull and deno test -A md ALL PASS

I am confused why it fails in the PR actions if I have the up to date code locally.

@pskfyi pskfyi force-pushed the test-readme-cross-platform branch from 4199572 to 93d18ab Compare May 8, 2023 03:34
@pskfyi
Copy link
Owner Author

pskfyi commented May 8, 2023

There was a compiler error introduced by the final commit because #49 added the return type of Promise<void> to evalCodeBlocks (meaning it should return nothing or undefined) but the final commit here returned the results of evaluateAll from earlier in the function. Updating the return type resolved the issue 👍

Copy link
Collaborator

@AustinArey AustinArey left a comment

Choose a reason for hiding this comment

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

Nice work thanks for the merge conflict fix.

@AustinArey AustinArey merged commit c67c2be into main May 8, 2023
@AustinArey AustinArey deleted the test-readme-cross-platform branch May 8, 2023 03:45
@pskfyi
Copy link
Owner Author

pskfyi commented May 8, 2023

Awesome 🎉

If you still have the failing code locally, you can run deno task test-readme and see that the evalCodeBlocks one fails. That's why it failed in CI in all 3 OS's.

Surprisingly, deno lint and deno fmt --check which we're running in CI don't surface the issue. I'm wondering if we should add a step to run deno check in Ubuntu, which does surface these sorts of issues. Right now if we had such an error in a file not imported in a readme example, or only imported in a readme example which uses no-eval, then the compiler error would not be surfaced in CI.

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.

2 participants