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: update main workflow #139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ObserverOfTime
Copy link
Contributor

.github/workflows/main.yml Outdated Show resolved Hide resolved
uses: tree-sitter/parse-action@v4
id: parse-files
with:
files: |-
examples/neovim/runtime/doc/*
# FIXME: these files should not have errors
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

@ObserverOfTime ObserverOfTime Oct 20, 2024

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.

@ObserverOfTime
Copy link
Contributor Author

I suppose this proves that the parser did not break due to any of the changes in #137.

@clason
Copy link
Member

clason commented Oct 20, 2024

Test failure is from #134. Not sure why tests passed on that PR but fail now.

@@ -17,6 +18,7 @@ on:
- test/**
- bindings/**
- binding.gyp
- .github/workflows/*
Copy link
Member

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
Copy link
Member

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.

Copy link
Contributor Author

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.

@dundargoc
Copy link
Member

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 tree-sitter/parser-test-action and independent to it added tree-sitter/parse-action and tree-sitter/setup-action/cli (which I assume is needed by tree-sitter/parse-action)?

@ObserverOfTime
Copy link
Contributor Author

tree-sitter/parser-setup-action is replaced by actions/checkout + tree-sitter/setup-action.
tree-sitter/parser-test-action@v2 no longer calls tree-sitter/parse-action automatically.

uses: tree-sitter/parser-test-action@v2
with:
test-library: ${{runner.os == 'Linux'}}
corpus-files: |-
test-rust: ${{runner.os == 'Linux'}}
Copy link
Member

@dundargoc dundargoc Oct 23, 2024

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?

Copy link
Contributor Author

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.

Copy link
Member

@dundargoc dundargoc left a 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'
Copy link
Member

@dundargoc dundargoc Oct 28, 2024

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

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.

4 participants