Submitted a rerolled patch and got a failed test based on a line in the test file $this->assertRaw('.setClickUrl("http://drupal7.dev/javascript%3A//example.com%3Ffoo%26bar")', 'Click URL is filtered for bad protocols and absolute.');

I'm going to assume that the drupal7.dev in there was accidental and replace it with example.com

CommentFileSizeAuthor
#33 not_passing_test-2846350-28.patch694 bytesmarcelovani
#26 not_passing_test-2846350-26.patch700 bytesmarcelovani
#19 not_passing_test-2846350-18.patch786 bytesmarcelovani
#17 not_passing_test-2846350-17.patch1.16 KBdorficus
#14 not_passing_test-2846350-14.patch1.2 KBdorficus
#13 not_passing_test-2846350-13.patch1.2 KBdorficus
#12 not_passing_test-2846350-12.patch785 bytesdorficus
#9 not_passing_test-2846350-9.patch791 bytesdorficus
#7 not_passing_test-2846350-7.patch726 bytesbleen
#2 not_passing_test-2846350-2.patch746 bytesdorficus

Comments

dorficus created an issue. See original summary.

dorficus’s picture

dorficus commented
StatusFileSize
new not_passing_test-2846350-2.patch746 bytes
dorficus’s picture

dorficus commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: not_passing_test-2846350-2.patch, failed testing.

dorficus’s picture

dorficus commented

I'm curious if this might be an issue of assertNoRaw vs assertRaw. The test string looks like something we _wouldn't_ want to see on a click URL, but I could definitely be mistaken. I was wrong. Nothing to see here

dorficus’s picture

dorficus commented

I think the error goes back to the domain. Since we can't be sure of what the testing domain is going to be, unless we can and I don't know it. Either way, I think it would be safer to remove the domain from the assertRaw call and check to see if the module filtered out the bad protocol. This will result in not testing for absolute, but it will test for filtering.

bleen’s picture

bleen commented
Status: Needs work » Needs review
StatusFileSize
new not_passing_test-2846350-7.patch726 bytes
dorficus’s picture

dorficus commented

@bleen can we pull in $base_url to test for absolute still?

dorficus’s picture

dorficus commented
StatusFileSize
new not_passing_test-2846350-9.patch791 bytes

The last submitted patch, 7: not_passing_test-2846350-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: not_passing_test-2846350-9.patch, failed testing.

dorficus’s picture

dorficus commented
Status: Needs work » Needs review
StatusFileSize
new not_passing_test-2846350-12.patch785 bytes

Trying without encoded characters

dorficus’s picture

dorficus commented
StatusFileSize
new not_passing_test-2846350-13.patch1.2 KB

I forgot to call the global in the last patch. This should do the trick!

dorficus’s picture

dorficus commented
StatusFileSize
new not_passing_test-2846350-14.patch1.2 KB

Bleh... someday I'll remember to check my work before pushing patches. Sorry for all the notifications!

The last submitted patch, 12: not_passing_test-2846350-12.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: not_passing_test-2846350-14.patch, failed testing.

dorficus’s picture

dorficus commented
Status: Needs work » Needs review
StatusFileSize
new not_passing_test-2846350-17.patch1.16 KB

The failure is generating this error in Jenkins AH00558: apache2: Could not reliably determine the server's fully qualified domain name, using 172.17.0.2. Set the 'ServerName' directive globally to suppress this message which seems to be an issue with Jenkins and not the DFP module. I think we have 2 options, you can either push forward with the fix as fixed despite the failed test, or we can only test for the filtered bad protocols and not absolute. I've attached a patch that only checks for the bad protocols filtered.

Status: Needs review » Needs work

The last submitted patch, 17: not_passing_test-2846350-17.patch, failed testing.

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented
Priority: Normal » Major
Status: Needs work » Needs review
StatusFileSize
new not_passing_test-2846350-18.patch786 bytes

My solution: Use assertPattern()
This will work with http/https on any domain
Changing the priority because this is breaking automated tests for all patches

Status: Needs review » Needs work

The last submitted patch, 19: not_passing_test-2846350-18.patch, failed testing.

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented

I reckon the automated test won't pass. The code that tests this patch has a broken test.
It needs to be tested locally and merged in.

  • bleen committed 232e333 on 7.x-1.x authored by marcelovani 
    Issue #2846350 by dorficus, marcelovani, bleen: Not passing test
bleen’s picture

bleen commented
Status: Needs work » Fixed

Woot!!

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented

Automated tests are still failing
https://dispatcher.drupalci.org/job/drupal_d7/7025/testReport/DFP/dfpDis...

But it passes on my local. No idea what is going on...

bleen’s picture

bleen commented
Status: Fixed » Needs work

Same here :(

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented
Status: Needs work » Needs review
StatusFileSize
new not_passing_test-2846350-26.patch700 bytes

I tried with different php versions locally and all work.
Its really hard to understand what is going on on that CI since there are no screenshots.
Trying changing the pattern...

Status: Needs review » Needs work

The last submitted patch, 26: not_passing_test-2846350-26.patch, failed testing.

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented

Again, the only way to test is by committing the patch, since CI uses the last commit
see https://dispatcher.drupalci.org/job/drupal_d7/7675/artifact/jenkins-drup...

Package operations: 3 installs, 0 updates, 0 removals
  - Installing drupal/entity_modified (1.2.0) Cloning 7.x-1.2
  - Installing drupal/ctools (1.12.0) Cloning 7.x-1.12
  - Installing drupal/dfp (dev-1.x 232e333) Cloning 232e3336e8

  • bleen committed 11b20ac on 7.x-1.x authored by marcelovani 
    Issue #2846350 by dorficus, marcelovani, bleen: Not passing test - take...
bleen’s picture

bleen commented

/me crosses fingers

bleen’s picture

bleen commented

still no dice :( .... BOOOO

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented
Status: Needs work » Closed (cannot reproduce)
marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented
Status: Closed (cannot reproduce) » Needs review
StatusFileSize
new not_passing_test-2846350-28.patch694 bytes

Clean urls is what was breaking it

  • bleen committed 82ddbca on 7.x-1.x authored by marcelovani 
    Issue #2846350 by dorficus, marcelovani, bleen: Not passing test
bleen’s picture

bleen commented
Status: Needs review » Fixed

Well done!!! 10 points to Griffendor

Status: Fixed » Closed (fixed)

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