mirror of
https://github.com/protocolbuffers/protobuf
synced 2026-08-26 02:23:14 -04:00
protobuf: improve staleness test regeneration to reduce confusion
This change updates `regenerate_stale_files.sh` to always execute a direct fix and refactors `staleness_test_lib.py` to support cleaner output during regeneration without sacrificing debugging info.
Motivation:
Automatic staleness tests are run to regenerate stale files on non-bazel environments (like GHA). When these files change, the tests "fail" to trigger regeneration. This causes constant confusion for developers as they look like test failures rather than "Working As Intended" regeneration triggers.
Details:
1. **`regenerate_stale_files.sh`**: Updated to always build the staleness tests via `bazel build` and execute them directly with `--fix --print-diffs`, avoiding the confusing "FAIL" output of `bazel test`. This script is primarily used by automation (like GHA) where this behavior is desired by default.
2. **`staleness_test_lib.py`**: Refactored diff generation into a shared helper `_GetDiffErrors`. Both `CheckFilesMatch` (testing) and `FixFiles` (fixing) use this helper. Added a `print_diffs` parameter to `FixFiles` so diffs are only printed when explicitly requested (e.g., in `regenerate_stale_files.sh`), preventing double-printing if run manually.
3. **Tone Improvement**: Updated the message for missing files during fixing to be less alarming ("Creating missing file" instead of "File does not exist").
### Output Comparison
**New Automation Output**:
Clean execution without `AssertionError` boilerplate, but preserves actionable diffs.
```
+ ./bazel-bin/upb/reflection/descriptor_upb_proto_staleness_test --fix --print-diffs
Creating missing file upb/reflection/stage0/descriptor_upb_proto.h
File upb/reflection/descriptor_upb_proto.c is out of date:
--- upb/reflection/descriptor_upb_proto.c.generated
+++ upb/reflection/descriptor_upb_proto.c
@@ -45,6 +45,7 @@
#include "upb/reflection/descriptor_upb_proto.h"
UPB_NOINLINE const upb_MiniTable* google_protobuf_FileDescriptorSet_msg_init() {
+ // Added a new field
return &google_protobuf_FileDescriptorSet_msg_init_table;
}
```
**Traditional Test Failure (Manual Runs)**:
Verbose, traditional test failure (for manual runs via `bazel test`).
```
+ bazel test upb/reflection:descriptor_upb_proto_staleness_test
...
FAIL: upb/reflection:descriptor_upb_proto_staleness_test (Exit 1)
================================================================================
AssertionError: Files out of date!
To fix run THIS command:
bazel-bin/upb/reflection/descriptor_upb_proto_staleness_test --fix
Errors:
File upb/reflection/descriptor_upb_proto.c is out of date:
--- upb/reflection/descriptor_upb_proto.c.generated
+++ upb/reflection/descriptor_upb_proto.c
...
```
PiperOrigin-RevId: 963179635
This commit is contained in:
parent
dce0d24702
commit
0fff4dec59
3 changed files with 42 additions and 15 deletions
|
|
@ -30,7 +30,8 @@ STALENESS_TESTS=(
|
|||
|
||||
# Run and fix all staleness tests.
|
||||
for test in ${STALENESS_TESTS[@]}; do
|
||||
${BazelBin} test $test "$@" || ./bazel-bin/${test%%:*}/${test#*:} --fix
|
||||
${BazelBin} build $test "$@"
|
||||
./bazel-bin/${test%%:*}/${test#*:} --fix --print-diffs
|
||||
done
|
||||
|
||||
# Generate C# code.
|
||||
|
|
|
|||
|
|
@ -55,6 +55,7 @@ class TestFilesMatch(unittest.TestCase):
|
|||
|
||||
|
||||
if len(sys.argv) > 1 and sys.argv[1] == "--fix":
|
||||
staleness_test_lib.FixFiles(config)
|
||||
print_diffs = "--print-diffs" in sys.argv
|
||||
staleness_test_lib.FixFiles(config, print_diffs=print_diffs)
|
||||
else:
|
||||
unittest.main()
|
||||
|
|
|
|||
|
|
@ -144,16 +144,52 @@ def _CopyFiles(file_pairs):
|
|||
copyfile(pair.generated, pair.target)
|
||||
|
||||
|
||||
def FixFiles(config):
|
||||
def _GetDiffErrors(missing_files, stale_files, is_fixing=False):
|
||||
"""Generates error messages/diffs for missing and stale files."""
|
||||
diff_errors = []
|
||||
for pair in missing_files:
|
||||
with open(pair.generated) as g:
|
||||
diff = "".join(
|
||||
difflib.unified_diff(
|
||||
[],
|
||||
g.read().splitlines(keepends=True),
|
||||
)
|
||||
)
|
||||
if is_fixing:
|
||||
diff_errors.append("Creating missing file %s:\n%s" % (pair.target, diff))
|
||||
else:
|
||||
diff_errors.append("File %s does not exist:\n%s" % (pair.target, diff))
|
||||
|
||||
for pair in stale_files:
|
||||
with open(pair.generated) as g, open(pair.target) as t:
|
||||
diff = "".join(
|
||||
difflib.unified_diff(
|
||||
g.read().splitlines(keepends=True),
|
||||
t.read().splitlines(keepends=True),
|
||||
)
|
||||
)
|
||||
if is_fixing:
|
||||
diff_errors.append("Updating stale file %s:\n%s" % (pair.target, diff))
|
||||
else:
|
||||
diff_errors.append("File %s is out of date:\n%s" % (pair.target, diff))
|
||||
return diff_errors
|
||||
|
||||
|
||||
def FixFiles(config, print_diffs=False):
|
||||
"""Implements the --fix option: overwrites missing or out-of-date files.
|
||||
|
||||
Args:
|
||||
config: the Config object for this test.
|
||||
print_diffs: whether to print diffs before fixing.
|
||||
"""
|
||||
|
||||
file_pairs = _GetFilePairs(config)
|
||||
missing_files, stale_files = _GetMissingAndStaleFiles(file_pairs)
|
||||
|
||||
if print_diffs:
|
||||
for error in _GetDiffErrors(missing_files, stale_files, is_fixing=True):
|
||||
print(error)
|
||||
|
||||
_CopyFiles(stale_files + missing_files)
|
||||
|
||||
|
||||
|
|
@ -167,20 +203,9 @@ def CheckFilesMatch(config):
|
|||
None if everything matches, otherwise a string error message.
|
||||
"""
|
||||
|
||||
diff_errors = []
|
||||
|
||||
file_pairs = _GetFilePairs(config)
|
||||
missing_files, stale_files = _GetMissingAndStaleFiles(file_pairs)
|
||||
|
||||
for pair in missing_files:
|
||||
diff_errors.append("File %s does not exist" % pair.target)
|
||||
continue
|
||||
|
||||
for pair in stale_files:
|
||||
with open(pair.generated) as g, open(pair.target) as t:
|
||||
diff = ''.join(difflib.unified_diff(g.read().splitlines(keepends=True),
|
||||
t.read().splitlines(keepends=True)))
|
||||
diff_errors.append("File %s is out of date:\n%s" % (pair.target, diff))
|
||||
diff_errors = _GetDiffErrors(missing_files, stale_files, is_fixing=False)
|
||||
|
||||
if diff_errors:
|
||||
error_msg = "Files out of date!\n\n"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue