This is an archive of the discontinued LLVM Phabricator instance.

[globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests
ClosedPublic

Authored by dsanders on Mar 31 2021, 4:36 PM.

Details

Summary

Have setUp() return the target machine so that the test has to call it to be able
to use it in the if(!TM)

Also, rename setUp() to avoid potential mix up with SetUp() from gtest

Diff Detail

Event Timeline

dsanders created this revision.Mar 31 2021, 4:36 PM
dsanders requested review of this revision.Mar 31 2021, 4:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2021, 4:36 PM
bogner accepted this revision.Mar 31 2021, 4:54 PM

The title/summary seems a bit confusing. The interesting part of this change is having the set up function return a target machine so it's impossible to miss - IMO renaming it is incidental. This seems reasonable to do, but I think it'd be better titled something along the lines of "Return a target machine from the test setup so that it's structurally required"

This revision is now accepted and ready to land.Mar 31 2021, 4:54 PM

The title/summary seems a bit confusing. The interesting part of this change is having the set up function return a target machine so it's impossible to miss - IMO renaming it is incidental. This seems reasonable to do, but I think it'd be better titled something along the lines of "Return a target machine from the test setup so that it's structurally required"

Sure I can swap them around. Thanks

dsanders retitled this revision from [globalisel][unittests] Rename setUp() to avoid potential mix up with SetUp() from gtest to [globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests.Mar 31 2021, 5:07 PM
dsanders edited the summary of this revision. (Show Details)

I wasn't sure whether setUp() was supposed to be SetUp() or not. Good to have that confusion go away!

Looks like phabricator noticed the revert but not the recommit. Here's the recommit:

bc21c7baaf00 [globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests

I forgot to update my local commit message to the phabricator one before committing the first time around