Remove low value tests.

[robdimsdale]

Summary

This PR removes the brats test listed below. I have two goals with this:

1. Removing tests that I think are low value. Typically this is tests that are slow/expensive (i.e. integration tests) that could be tested at the unit test level, as well as tests that I think are philosophically pointless (like negative tests) and therefore add cognitive load to a developer regardless of how quickly or slowly they run.
2. Making BRATS fast enough to tack on to the end of an existing integration/specs run rather than requiring a whole separate environment. The cost of this is fairly minimal - adding 5-20 mins onto the end of the existing integration test job - but the savings are significant, as it means not waiting for a shepherd pool env (and not encountering flakiness assigning/releasing it). As a data point, typically each brats tests takes 2-3 minutes, so this PR will save about 10-15 minutes (in the worst case, as not all buildpacks run all brats tests) over just inlining the brats as-is.

List of tests removed, with justification

• DeployingAnAppWithAnUpdatedVersionOfTheSameBuildpack: We cover this with a unit test. I do not think we also need an integration test
• StagingWithBuildpackThatSetsEOL: Not all dependencies have deprecation dates, and we have unit tests to cover this when dependencies do have deprecation dates.
• StagingWithADepThatIsNotTheLatestConstrained/StagingWithADepThatIsNotTheLatest: I do not think this provides much value to customers, and we have unit tests to cover this.
• DeployAnAppWithSensitiveEnvironmentVariables: I think these sorts of tests are unhelpful. There are an infinite number of negative test cases we can run - validating that the buildpack does perform a specific action is best tested at the unit level (if at all).

Future work

I think we could get to the point where we don't need to run brats at all, because we can inline the remaining tests into the integration test for each buildpack:

• There is one actual test remaining (the .profile test) which should be fairly easily to port into each buildpack's integration test suite and thus remove from brats.
• The remaining test harness - run for all dependency versions - could be turned into some helper functions and the testing itself moved into each buildpack's integration test suite, again, allowing for its removal from brats.

[ForestEckhardt]

• UnbuiltBuildpack: I do not think customers ever want to use the unbuilt version of a buildpack. They either use the buildpacks we create or (occasionally) recompile them themselves.

My understanding is that this test a user referencing a github URI in their build command which is a valid workflow. In this case the buildpack is built during the build which is something I think would still need to be tested.

[robdimsdale]

My understanding is that this test a user referencing a github URI in their build command which is a valid workflow. In this case the buildpack is built during the build which is something I think would still need to be tested.

@ForestEckhardt is this a workflow we actually want to support though? I don't think any of our consumers actually do this.

[robdimsdale]

Ok @ForestEckhardt i put back the Unbuilt test.

[sophiewigmore]

I think I'm good with this, but wanted to ask about DeployAnAppWithSensitiveEnvironmentVariables. I totally get what you mean about not having negative test cases. However, this kind of seems like an important one - not writing credentials to an app droplet? Seems like a security issue I wouldn't want to mess with. It must've mattered at some point because we added a test for it. I'm fine to rip it out if we're able to ascertain that we are unit testing this in some capacity.

[robdimsdale]

@sophiewigmore

I'm fine to rip it out if we're able to ascertain that we are unit testing this in some capacity.

We're not unit testing it, because it's not possible to unit test that the droplet has the absence of something until the droplet is created. We could test bits and pieces, but even at the unit test I'd argue that a negative test is not appropriate.

It must've mattered at some point because we added a test for it.

The test was added 7 years ago, when this library was extracted from whatever came before it. I assume it existed long before that too, i just don't know where to look to continue the thread.

Regardless of exactly when it was introduced - empirically we haven't ever come close to accidentally doing this (because we've never see this test fail). It seems like a huge stretch to me that after all these years we would suddenly introduce a feature that writes credentials to a disk after we deleted the test that would prevent this.

Some hypothetical examples of other negative tests (slightly more unlikely to drive the point home):

• we only write the libraries to one location - if we accidentally wrote the libraries to multiple directories the droplet would be N times larger.
• We don't include dependencies that aren't in the manifest - if we accidentally downloaded old dependencies the droplet would be bigger and get flagged a security risk.
• We don't modify the app code (for interpreted languages like python and ruby) - if we changed the app code customers wouldn't trust the buildpacks.

None of these happen, and we don't have tests for them. We don't feel the need to add tests because we think it's really unlikely that these outcomes could ever happen. I feel the same for the DeployAnAppWithSensitiveEnvironmentVariables test.

Again, if you feel really strongly about this we can keep it in. But these tests take time, and i really want to start being more deliberate about what tests we are keeping, and way. Integration test should describe the things that the buildpack does, not the things it doesn't do.

---

A couple more topics that I think we'll be coming back to soon:

• I don't think integration tests should be an ever-growing list of "things we did once and want to ensure don't happen again".
• I don't think we need to run every permutation of everything all the time (e.g. every dependency in brats; every commit against both switchblade and CF environments)

[sophiewigmore]

@robdimsdale makes sense that we can't unit test that, thanks. I just wanted to make sure we're not just saying "this stuff is old and probably not useful, lets just remove it". Having a thoughtful discussion about why this stuff can be removed without too much worry was the goal, and I feel like you've delivered here. Your logic about why we should remove this/it's not super helpful makes sense to me!

Btw

I don't think integration tests should be an ever-growing list of "things we did once and want to ensure don't happen again".

Yes