Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
Issue tags: +Novice, +API clean-up, +Config novice
FileSize
3.16 KB
3.16 KB
3.16 KB
3.16 KB

Let's see how bots will pass...
Extended test would be reverted in #1893442-6: Move BlockStorageController::loadByProperties() into ConfigStorageController

andypost’s picture

Changed comparison to allow debug also I think this easy to understand

andypost’s picture

Was wrong patch (locally pass randomly)

Status: Needs review » Needs work

The last submitted patch, 1981418-voc-sort-2.patch, failed testing.

andypost’s picture

alphawebgroup’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -API clean-up, -Config novice

#3: 1981418-voc-sort-2.patch queued for re-testing.

alphawebgroup’s picture

#3: 1981418-voc-sort-2.patch queued for re-testing.

alphawebgroup’s picture

#3: 1981418-voc-sort-2.patch queued for re-testing.

alphawebgroup’s picture

#3: 1981418-voc-sort-2.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +API clean-up, +Config novice

The last submitted patch, 1981418-voc-sort-2.patch, failed testing.

alphawebgroup’s picture

Status: Needs work » Needs review
FileSize
4.67 KB

re-rolling

Status: Needs review » Needs work
Issue tags: -Novice, -API clean-up, -Config novice

The last submitted patch, 1981418-voc-sort-3.patch, failed testing.

javisr’s picture

Status: Needs work » Needs review

#11: 1981418-voc-sort-3.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +API clean-up, +Config novice

The last submitted patch, 1981418-voc-sort-3.patch, failed testing.

andypost’s picture

Issue tags: +Needs reroll

patch needs re-roll

dsdeiz’s picture

Status: Needs work » Needs review
FileSize
4.09 KB

Re-rolling. Didn't test locally thought. Can't get simpletest to work. I'll probably give the bot a try.

OT: How do I create an interdiff when re-rolling patches?

Status: Needs review » Needs work

The last submitted patch, 1981418-16-voc-sort.patch, failed testing.

pcambra’s picture

Ignore this patch

pcambra’s picture

Status: Needs work » Needs review
FileSize
4.08 KB
974 bytes

Here's a re-roll with a minor fix on taxonomy load, it should pass now.

Status: Needs review » Needs work

The last submitted patch, 1981418-19-voc-sort.patch, failed testing.

pwieck’s picture

Issue tags: -Needs reroll

Removing tag. Still applies to current head

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -API clean-up, -Config novice

#19: 1981418-19-voc-sort.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +API clean-up, +Config novice

The last submitted patch, 1981418-19-voc-sort.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.72 KB

That failing assertion is useless. It tests a behavior that doesn't exist. It just arbitrarily sorts and tests the arbitrary sort.

andypost’s picture

Removal of the test means that we have no tests for default sorting order for configurables ConfigEntityListController::load()

#1893442: Move BlockStorageController::loadByProperties() into ConfigStorageController added the check for load_by_properties()

Also that means that out default sorting for configurables is fragile

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyUnitTest.phpundefined
@@ -120,20 +120,12 @@ function testTaxonomyVocabularyLoadMultiple() {
-    // Fetch all of the vocabularies using taxonomy_vocabulary_load_multiple().
-    // Confirm that the vocabularies are ordered by weight.
-    $vocabularies = taxonomy_vocabulary_load_multiple();
-    taxonomy_vocabulary_sort($vocabularies);

Patch in #3 shows that this leads to random failures, so I cant rtbc it

tim.plunkett’s picture

The testing for default ordering belongs with config_test then, lets move the coverage there.

vijaycs85’s picture

Issue tags: -Novice, -Config novice

Updating tags, as it doesn't sounds like novice issue anymore.

andypost’s picture

The function is not used and sorting happens in list controller so re-roll of last patch

xjm’s picture

No real need for a change record here since the function didn't exist in D7.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.