-
Notifications
You must be signed in to change notification settings - Fork 13
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: update main workflow #139
base: master
Are you sure you want to change the base?
Conversation
.github/workflows/main.yml
Outdated
uses: tree-sitter/parse-action@v4 | ||
id: parse-files | ||
with: | ||
files: |- | ||
examples/neovim/runtime/doc/* | ||
# FIXME: these files should not have errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we fixed all these files; please try not excluding them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, looks like this includes the parsing errors from injections (which core tests ignore). Still, the list looks like it has to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the files except for one fail to parse on Windows. I'll disable it for now.
I suppose this proves that the parser did not break due to any of the changes in #137. |
Test failure is from #134. Not sure why tests passed on that PR but fail now. |
84d23e3
to
e157dd0
Compare
@@ -17,6 +18,7 @@ on: | |||
- test/** | |||
- bindings/** | |||
- binding.gyp | |||
- .github/workflows/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh kinda wonder how worth using these paths are since the jobs are so fast anyway.
uses: tree-sitter/[email protected] | ||
with: | ||
node-version: 20 | ||
- name: Clone repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick/personal preference/not a blocker: I personally tend to dislike adding name
to common actions because it's usually self-explanatory what the action does. It gives off
for i in range(4) # i is an iterator
comment vibes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes the job logs consistent.
I think your commit/PR message could use more effort if I'm being honest. I'm having some trouble understanding what is happening here. I'm assuming you fixed incorrect input to |
|
uses: tree-sitter/parser-test-action@v2 | ||
with: | ||
test-library: ${{runner.os == 'Linux'}} | ||
corpus-files: |- | ||
test-rust: ${{runner.os == 'Linux'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this. Why only linux? Is it to save CI time? Is it because it's safe to assume that if it works for one system it will work for all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I don't mind setting it to either false
or true
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here look good to me. I glanced at the workflows/actions that are used and they look pretty complicated and convoluted, but I guess as long as we don't need to maintain them then that's alright?
test-rust: ${{runner.os == 'Linux'}} | ||
- name: Parse sample files | ||
uses: tree-sitter/parse-action@v4 | ||
if: runner.os != 'Windows' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if this or something equivalent was here before the PR, but this seems weird to me. If some of the files can't correctly be parsed on windows (but can on linux), then to me that still indicates that there is a problem we shouldn't ignore. Right now we cannot prevent regressions on windows since we don't test it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could make it continue-on-error
on Windows so you can still check the summary for failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That still won't solve the problem of regressions, and would also condition people into accepting CI failures so I don't think that's optimal. I still don't understand the benefit of ignoring the failures on windows if I'm being honest.
See https://github.com/tree-sitter/workflows#ci-workflow