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

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Status: Needs review » Needs work

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

joelpittet’s picture

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

Status: Needs work » Needs review
FileSize
2.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

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.

Cottser’s picture

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

Status: Needs work » Reviewed & tested by the community
FileSize
2.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

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...
Cottser’s picture

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

Status: Fixed » Closed (fixed)

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