Page MenuHomePhabricator

Delete unstable integration tests
AbandonedPublic

Authored by jroelofs on Apr 18 2017, 10:07 AM.

Details

Reviewers
arphaman
Summary

These tests are breaking when tested under the upstream 3.8.1 release + the 10.12 / 16C58 sdk. They use headers from the host SDKs, so they are not stable with respect to adding new language features, such as class @properties.

Regression tests should not depend at all on the state of the host system. If these tests are still useful, they ought to live somewhere else (test-suite?).

Diff Detail

Event Timeline

jroelofs created this revision.Apr 18 2017, 10:07 AM
arphaman edited edge metadata.Apr 18 2017, 11:52 PM

Thanks for working on this!

We had some more discussion about these tests yesterday, and we would prefer to keep them actually. We might want to use something like REQUIRES instead, but right now we are still not sure what to check for.

I think you'd have to check for a maximum host SDK version, and then bump it upstream every time there's a new one thats known to work with the then-current trunk.

It can't be done based on language features because by definition, one cannot know what features are going to be added in the future.

All of that being said, we'd still have a situation where the test means different things on different host systems purely based on what's installed on that system (which is not a good thing IMO).

Ping.

@arphaman I'd like to remove them, absent a concrete plan to fix this from folks with a vested interest in them (i.e. you).

rsmith added a subscriber: rsmith.May 11 2017, 5:04 PM

Clang's regression test suite is not a sensible home for these tests. We should have a home and a plan for system-specific integration tests, but this is not it. Perhaps this should instead be a part of LNT / the test-suite project?

@jroelofs
We'd be ok with tying the test to a specific SDK and maintaining that tie (e.g. bumping it upstream).
I was planning to investigate how to make the SDK version checks work, but I don't have the time ATM. Can I get back to you in a week?

Sure. That'd be needed whether they stay, or get moved to test-suite.

@jroelofs

I've managed to put together a solution that adds REQUIRES to the tests that ensure they only work with the latest SDK (10.12).

WDYT?

arphaman added a comment.EditedMay 19 2017, 4:17 AM

I do agree that these tests would be better off in another place, but we currently don't have the bandwidth to facilitate that move.

@arphaman Those patches seem reasonable.

Ok, I will commit them sometime this week then.

Sorry for the delay, was on vacation. Committing today.

Committed r304541 & r304542.

jroelofs abandoned this revision.Jul 29 2020, 1:59 PM