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

Create a failure test rule #301

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

FaBrand
Copy link

@FaBrand FaBrand commented May 27, 2021

When testing for analysis-phase-failures in rules this rule provides otherwise necessary boilerplate that simply allows to test for correct Error messages

I discovered this reoccuring rule while testing some of my rules and thougt it might be useful to others as well.

@google-cla google-cla bot added the cla: yes label May 27, 2021
@FaBrand FaBrand force-pushed the failure_test branch 2 times, most recently from d3ccd95 to dbfe808 Compare May 27, 2021 10:00
@FaBrand
Copy link
Author

FaBrand commented Jun 1, 2021

@brandjon @tetromino Do you have any feedback on this?

@FaBrand
Copy link
Author

FaBrand commented Sep 24, 2021

Bump :)

Copy link
Collaborator

@tetromino tetromino left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. I agree with the idea and the code; I've searched and found a few independent reimplementations of the exact same logic, so I think the new rule would be useful.

My comments are not about the code, but about all the boring parts (wording, documentation, and tests):

  • this is not a general failure test rule - it's rule for testing analysis-time failure. Therefore, it should be named analysis_failure_test (and the file named accordingly)
    • File docstring should be more clear about this - e.g. something like "A test verifying that another target cannot be analyzed"
  • You need a docstring for the rule function and all attributes; you can use analysis_test.bzl as an example. The docstring must include a usage example. Otherwise, how could a user trying out this rule know that, for instance, they need to set the target_under_test attribute?
  • You need a test for the new test. You'd want to verify that for a target which fails analysis, analysis_failure_test succeeds. You'd want to verify that for a target which passes analysis, analysis_failure_test fails at bazel test time (not at analysis time). This means you need shell tests which launch bazel (a bit like tests/analysis_test_test.sh does).

Nits:

  • Copyright year is 2021 :)

@FaBrand FaBrand force-pushed the failure_test branch 5 times, most recently from 43ce840 to 5d582d6 Compare September 28, 2021 12:59
@FaBrand
Copy link
Author

FaBrand commented Sep 28, 2021

@tetromino Thanks for the detailed feedback :)
I updated the change with your findings and also added the stardoc target for the new rule and generated the docs.
However i'm not sure if i have much leverage (or maybe missed something) on generating good documentation there.
e.g. the target_under_test or the rule docs attribute can't be set through the analysistest.make and stardoc generates empty defaults. The module docstring does not seem to be included as well.

@tetromino
Copy link
Collaborator

Sorry for very delayed review; I'm on leave. But as for the generated docs problem, I realized that we were using a very old Stardoc release via the (deprecated) Federation repo. We'll fix that by #327

@FaBrand FaBrand force-pushed the failure_test branch 2 times, most recently from c74d27a to 59524ff Compare March 16, 2022 16:39
@FaBrand
Copy link
Author

FaBrand commented Mar 16, 2022

@tetromino Sry for the even later reaction :/

I fixed the docs and squashed to commits for a cleaner history

@FaBrand FaBrand force-pushed the failure_test branch 3 times, most recently from a119b93 to 4c3ea0b Compare March 16, 2022 16:47
@FaBrand FaBrand force-pushed the failure_test branch 2 times, most recently from d737b3f to effb387 Compare October 24, 2022 09:23
When testing for analysis-phase-failures in rules this rule provides
otherwise necessary boilerplate that simply allows to test for correct
Error messages
@FaBrand
Copy link
Author

FaBrand commented Oct 27, 2022

Kind of forgot this PR 🙈 I updated everything to the latest version. Could you please update your review @tetromino ?
Or is ownership handled differently now @brandjon @comius @hvadehra @c-mita ?

@FaBrand
Copy link
Author

FaBrand commented Apr 26, 2023

ping @brandjon @comius @c-mita

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants