search

keith / buildifier-prebuilt

A bazel toolchain for using prebuilt binaries for buildifier and buildozer
MIT License
35 stars 13 forks source link

Test: new test cases showing existing issues on windows #90

Open peakschris opened 2 months ago

peakschris commented 2 months ago

This PR adds unit tests for buildifier that fail to demonstrate the current issues with Windows.

Because running fix modifies the source tree, I set them up as bazel shell tests so that they run in temporary workspaces.

You can add this command to CI: bazel test //tests/buildifier/... --enable_runfiles

Although the tests must run with runfiles, the sub-workspaces that they validate run in both runfiles and non-runfiles mode.

peakschris commented 2 months ago

It would be nice to re-enable the existing integration tests for windows on CI. However, rules_bazel_integration test is currently broken on windows - around half of the test cases are failing. Part of this might be its dependency on buildifier-prebuilt! I can have a look at that at some point, but for now I suggest we use these bazel shell tests to add coverage that works on all platforms, and I'll look at fixing rules_bazel_integration_test as a followup once my windows fixes here are released.

peakschris commented 2 months ago

@keith I've pushed a fix for ubuntu. For windows, could you add --enable_runfiles to the test command? That is required.

peakschris commented 2 months ago

Actually, I've fixed the new test so it can be used in norunfiles mode.

@keith, could you rerun CI please?

keith commented 2 months ago

started

peakschris commented 2 months ago

I've improved the test cases. I'd expect //tests:buildifier:buildifier_test to fail on both linux and windows. The linux failures are due to buildifier check/fix having some issue in norunfiles mode.

@keith could you rerun CI?

These are the messages reported from inside //tests:buildifier:buildifier_test On windows: ** 2 / 6 tests passed. ***** There were errors. ****

On linux: ** 4 / 6 tests passed. ***** There were errors. ****

cgrindel commented 2 months ago

I just approved the CI run.

peakschris commented 2 months ago

Thank you. That is failing as expected. I've merged the latest state of this PR into #89 so we can evaluate those results. Please do whatever you see fit in terms of closing these separately or just as #89.

peakschris commented 2 months ago

@cgrindel thank you, windows 👍 , issue with bazel 5 on linux, I've submitted a fix, could you rerun?

cgrindel commented 2 months ago

@peakschris I don't see an Approve button for CI. DId you push your changes?

peakschris commented 2 months ago

@cgrindel it auto-ran. This CI is now good; passing linux, failing on windows. The windows failures are addressed by #89.

brentleyjones commented 2 months ago

@peakschris I don't see an Approve button for CI. DId you push your changes?

I pushed it.