Just for giggles and grins.

#2 2108809-2.patch2.34 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 59,018 pass(es). View
standard.patch582 byteswebchick
FAILED: [[SimpleTest]]: [MySQL] 58,539 pass(es), 2 fail(s), and 0 exception(s). View


Status: Needs review » Needs work

The last submitted patch, standard.patch, failed testing.

beejeebus’s picture

Status: Needs work » Needs review
2.34 KB
PASSED: [[SimpleTest]]: [MySQL] 59,018 pass(es). View

so this works for me locally.

webchick’s picture

Status: Needs review » Needs work

That works, but is only testing exporting/importing on the *same* site. We need to test it *across* sites, since that's how we uncover fun stuff like #1969800: Add UUIDs to default configuration and #2108419: user module overwrites role actions breaking config sync.

webchick’s picture

Issue tags: +Configuration system


beejeebus’s picture

i thought about this a bit this morning, and i don't think #3 gets at the real issue.

there are at least two ways to set up different environments. one way is to do two separate installs with the same profile, modules etc. let's call that "TwoInstalls".

another way is to do one install, dump and reimport the db with a new name and copy over all the files. let's call that "CopyInstalls".

so here's the thing - with our current design, there should be no difference between the two. the set of installed config files in both those scenarios should be the same, and if you export from one and import into the other, Drupal should tell you to stop wasting it's time, because there's nothing to do.

if the TwoInstalls method violates that assertion, then that's the bug we need to focus on. if we can't fix things so that TwoInstalls behaves just like CopyInstalls, we're in trouble, because we'll have to create config deploy tests for both methods, which is insane.

in short - i think we need to change the test that recently went in to assert that without making any changes, an export from one install will tell you "there's nothing to do" when importing to the other.

ianthomas_uk’s picture

beejeebus and I spent a while discussing this on IRC, but I admit I didn't totally understand the argument.

My understanding:
#2 tests that if you change a slogan, but then re-import the original slogan, then you will end up with the original slogan.
At the very least this test needs a ->get() based assertion between the two \Drupal::config()->save(); calls, otherwise what is the point of the first call?

I think beejeebus is arguing that if the slogan is wrong (i.e. doesn't match config), and it doesn't matter why that happened. Therefore, you might as well keep setting and checking config based on a single install, rather than doing separate installs.

The original test is based on two separate installs each generating a random number, but when and why does this happen? It is much more likely that you'll run multiple installs, each generating the same identifiers for standard content, until you choose to switch to a canonical database.

chx’s picture

As for #5 yes absolutely we need to add an assert to that extent. If I remember correctly, I saw locale.settings changing when I worked in this issue last, but it was so long ago, I am not sure.

However, I think that we are doing a huge disservice to ourselves if the TwoInstalls vs CopyInstall scenario doesn't work the same. Copying an install is not a trivial matter, after all: it requires some database tool. If we could avoid that... it'd be great.

sun’s picture

Issue summary: View changes
Status: Needs work » Closed (duplicate)

#3 is covered by dedicated Site UUID tests in the meantime.

The original patch here introduces a test that already exists in HEAD, so that appears to be obsolete, too.

Therefore, and because there is no issue summary and thus no way to validate whether there might have been a more complex underlying goal, this issue appears to be obsolete, since resolved by other issues already.

Lastly, FWIW, no (new) test in core should use $profile = 'standard'; — any test that requires the Standard profile to assert what it wants to assert is not a valid test. That is, because the human English translation of doing so is:

The problem only occurs with Standard profile, but we have no idea what's going on. Hence, let's just use the Standard profile in this test, so we do not have to figure out the actual root cause.

Prior to the 8.0 stable, there should no longer be any test in core that is using the Standard profile. (which will also have a huge impact on the total core test suite time/performance.)

If time/energy permits, we will remove the $profile property altogether from Simpletest test classes, in order to prevent bogus test coverage to exist in the first place.

tim.plunkett’s picture

We'll still have
"Tests that the standard profile mappings are set and exposed as expected."

But agreed otherwise.

sun’s picture

Indeed, those RDF tests are a bit special. Although I still don't get why that isn't auto-generated default configuration... Unless I misunderstand the situation, then this means that installing Comment module + RDF module in any other installation profile than Standard means that actually no RDF output will be generated. (weirdo?)

In any case, even a test like that can be written in a way to avoid installing the full Standard profile. Our new config schema validation tests are a leading example — they are validating the config schema of all extensions that are discovered in the filesystem. Just like config schema, RDF tests are shooting for machine-readable aspects only. Hence, such a test could totally use DUTB, too. :)

Frankly, all of the remaining Standard profile tests only exist, because I ran out of steam in the original #1373142: Use the Testing profile. Speed up testbot by 50% — but we are already tracking and resolving the remaining offenders in #1595028: [meta] Convert tests using Standard profile to use Testing profile instead

Additionally created #2203057: Remove $profile property from WebTestBase