Page MenuHomePhabricator

[Bash-autocompletion] Add comment to test so that it is easier to fix

Authored by yamaguchi on Aug 1 2017, 10:45 PM.



clang/test/Driver/autocomplete.c is a test for --autocomplete, and this
test might break if people add/modify flags or HelpText. So I've add
comment for future developers so that they can fix this file according
to the change they had made.

Diff Detail


Event Timeline

yamaguchi created this revision.Aug 1 2017, 10:45 PM
ruiu edited edge metadata.Aug 1 2017, 10:52 PM

I wonder if this test is actually fragile. How can you break this test by adding a new flag?

3 ↗(On Diff #109274)

Keep it in 80 cols.

yamaguchi updated this revision to Diff 109277.Aug 1 2017, 11:10 PM

Update diff.

ruiu added a comment.Aug 1 2017, 11:15 PM

So, does this comment actually make sense? Looks like you cannot break this test by adding or modifying flags or by changing HelpText. (Theoretically, it'll break if you remove -fsyntax-only, for example, but that is not realistic.)

yamaguchi added a comment.EditedAug 1 2017, 11:17 PM

This test will break for instance, when someone add new value to mrelocation-model because values has to be shown in row 78- 83 order, or when someone made a new flag which start with -Wno-invalid-pp- because in row 96 only -Wno-invalid-pp-token is expected for this prefix.

teemperor accepted this revision.Aug 1 2017, 11:29 PM

E.g. If you add a flag that lands in between the results it breaks. Add -Wmajor-new-feature and the test goes red. Same with the flag results or changing the help test for -std=.

We could fix that by making the test less aggressive but it's already too forgiving right now (It actually passes when you completely break the autocomplete filtering feature and just return everything).

Maybe we need a smart solution(TM) in the future where we solve this in a more stable way while still actually testing the shipped clang binaries autocomplete feature.

2 ↗(On Diff #109274)

"test had broke" -> "test broke"

This revision is now accepted and ready to land.Aug 1 2017, 11:29 PM
yamaguchi updated this revision to Diff 109281.Aug 1 2017, 11:57 PM

Modified comment.

yamaguchi updated this revision to Diff 109283.Aug 2 2017, 12:01 AM

Fixed typo.

ruiu accepted this revision.Aug 2 2017, 12:08 AM


This revision was automatically updated to reflect the committed changes.