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

Fix #4140: Allow Bazel based tests to run with string test input #4170

Merged
merged 1 commit into from
Oct 12, 2021

Conversation

davidburstromspotify
Copy link
Contributor

It does not fix test cases that use resource files from the project, but it's a start at least.

@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #4170 (fa5f41e) into main (8b31b44) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #4170   +/-   ##
=========================================
  Coverage     83.44%   83.44%           
  Complexity     3191     3191           
=========================================
  Files           465      465           
  Lines          9119     9119           
  Branches       1779     1779           
=========================================
  Hits           7609     7609           
  Misses          572      572           
  Partials        938      938           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b31b44...fa5f41e. Read the comment docs.

@davidburstromspotify
Copy link
Contributor Author

Is the Windows build flaky? I have a hard time seeing that it could relate to my patch.

@BraisGabin
Copy link
Member

It is. Don't worry about it.

@davidburstromspotify
Copy link
Contributor Author

@chao2zhang Thanks for the review! I'm don't have write access to merge it, could you please?

@chao2zhang
Copy link
Member

Usually, we require 2 reviewers for merging PRs. Since @BraisGabin has already read this PR, I will wait for him to merge.

@BraisGabin
Copy link
Member

I suppose that this is OK to merge. It's on the test code and it doesn't break our tests.

But I'm worried that in the future someone could revert this change without knowing this was intentional. Would it be possible to create a test so we can avoid the regression?

@chao2zhang
Copy link
Member

Adding comments seems sufficient to me. We don't officially support Bazel builds so it is hard to verify in the detekt codebase.

It does not fix test cases that use resource files from the project, but it's a start at least.
@davidburstromspotify
Copy link
Contributor Author

I've added an in-line explanation why this is needed. Yes, ideally there should be a test cases for this. Two options for this is to add a Bazel driven build, or to introduce a special Gradle Test task that only has JARs on the classpath. Not sure if it is worth the maintenance overhead for that.

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.

3 participants