testMultipleLibrariesAreNotLoaded is getting 404 it seems but it is still passing. It seems like the test case is not really testing anything.

CommentFileSizeAuthor
#8 2806733-8.patch2.05 KBjoelpittet
#5 2806733-5.patch2.05 KBjoelpittet
#2 2806733-1.patch584 bytesjoelpittet

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
StatusFileSize
new 2806733-1.patch584 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 2806733-1.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Component: javascript » asset library system
Priority: Normal » Major

Ok something must have happened but this test no longer tests anything because it's returning a 404. Bumping to major and changing to the asset library system

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new 2806733-5.patch2.05 KB

Ok it needs node and views modules enabled so I added more tests to make sure this is working as expected.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Title: testMultipleLibrariesAreNotLoaded » testMultipleLibrariesAreNotLoaded is not asserting anything
Category: Bug report » Task
Priority: Major » Normal
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Updated the issue summary to explain better what are we fixing with this change.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Thanks for the ping :)

  1. +++ b/core/modules/system/src/Tests/Render/AjaxPageStateTest.php
@@ -77,22 +84,16 @@ public function testHtml5ShivIsNotLoaded() {
-   * ajax_page_state[libraries] should be able to support multiple libraries
+   * The ajax_page_state[libraries] should be able to support multiple libraries

    I'm not sure this is an improvement :) 

  2. +++ b/core/modules/system/src/Tests/Render/AjaxPageStateTest.php
@@ -102,6 +103,17 @@ public function testMultipleLibrariesAreNotLoaded() {
+      'The html5shiv library from core should be includes in loading.'
...
+      'The drupalSettings library from core should be includes in loading.'

    Minor: I think these should say 'included' instead of 'includes'.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2806733-8.patch2.05 KB

@Cottser, 1) Was adding "The" to fix code sniffer complaining about starting with a capital letter. 2) That make sense, thanks for catching that.

Resetting this to RTBC because I've addressed (2), and (1) does fix things, though doesn't improve much. Feel free to remove that change on commit if you disagree with adding 'The' for code sniffer's sake.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed dc649db to 8.3.x and b9cf6e7 to 8.2.x. Thanks!

  • alexpott committed dc649db on 8.3.x 
    Issue #2806733 by joelpittet: testMultipleLibrariesAreNotLoaded is not...

  • alexpott committed b9cf6e7 on 8.2.x 
    Issue #2806733 by joelpittet: testMultipleLibrariesAreNotLoaded is not...
star-szr’s picture

star-szr
he/him
Primary language English
commented

@joelpittet regarding code sniffing: makes sense, cool :) thanks!

Status: Fixed » Closed (fixed)

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