Open Bug 939755 Opened 6 years ago Updated 2 years ago

With httpd.js we sometimes don't get the most recent version of the file

Categories

(Testing :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: julienw, Assigned: leni1, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #939729 +++

This bug is to apply the changes from bug 939729 to moz-central's httpd.js.
Hey Julien, let me know if I can help on this issue :-)
Hey Yuren,

please do, I haven't tried to do it on upstream's httpd.js yet. And maybe we need to introduce a "filter" concept instead of hacking inside the main file.
Whiteboard: [good first bug][mentor=jmaher][lang=javascript]
Mentor: jmaher
Whiteboard: [good first bug][mentor=jmaher][lang=javascript] → [good first bug][lang=javascript]
Whiteboard: [good first bug][lang=javascript] → [good first bug][lang=js]
apparently this is to ensure that the patch from bug 939729 is applied to all versions of httpd.js.
Applied the changes from bug 939729 to addon-sdk/source/test/addons/e10s-content/lib/httpd.js.

changed addon-sdk/source/test/addons/e10s-content/lib/httpd.js
Comment on attachment 8882547 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file

Started with the file addon-sdk/source/test/addons/e10s-content/lib/httpd.js.
The files addon-sdk/source/test/addons/content-permissions/httpd.js and addon-sdk/source/test/addons/content-script-messages-latency/httpd.js had the following message at the top: 

/*
*  NOTE: do not edit this file, this is copied from:
*  https://github.com/mozilla/addon-sdk/blob/master/test/lib/httpd.js
*/

In such a case how would I make the changes to these files, since PRs aren't accepted on GitHub?
Flags: needinfo?(jmaher)
Attachment #8882547 - Flags: review?(jmaher)
May I be assigned to the bug please? :)
Comment on attachment 8882547 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file

Review of attachment 8882547 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for this patch!  Submitting patches like this is just fine vs github.  The only reason for a r- is the extra whitespace.

::: addon-sdk/source/test/addons/e10s-content/lib/httpd.js
@@ +2654,5 @@
>      }
>      else
>      {
> +        var lastModified;
> +        

nit: there is extra whitespace here on the blank line, it should just be a newline, not a line with spaces/tabs.

@@ +2660,5 @@
>        {
> +          lastModified = toDateString(file.lastModifiedTime);
> +          response.setHeader("Last-Modified", lastModified, false);
> +      }
> +      catch (e) { 

extra space after {

@@ +2661,5 @@
> +          lastModified = toDateString(file.lastModifiedTime);
> +          response.setHeader("Last-Modified", lastModified, false);
> +      }
> +      catch (e) { 
> +          /* lastModifiedTime threw, ignore */ 

extra space after */

@@ +2679,5 @@
> +        dumpn(">>> file was not modified, returning a 304 code");
> +        response.setStatusLine("1.1", 304);
> +        return;
> +      }
> +       

the last 5 blank lines have extra spaces/tabs.
Attachment #8882547 - Flags: review?(jmaher) → review-
Leni, thanks for your work on this- I will be on holiday, :ahal can help with reviews and and next steps on this bug or other bugs- he is a great mentor and we work together often!
Flags: needinfo?(jmaher)
Hey, given I'm the original author of the patch in Bug 939729 and given the patch isn't very different to what I did, is it possible to have both Leni and my name on this patch ? If that's not possible with hg at least please quote me in the commit log :) Thanks
(In reply to Julien Wajsberg [:julienw] from comment #9)
> Hey, given I'm the original author of the patch in Bug 939729 and given the
> patch isn't very different to what I did, is it possible to have both Leni
> and my name on this patch ? If that's not possible with hg at least please
> quote me in the commit log :) Thanks

Hmmm... Sure. Don't see why not. How would I quote you in the commit log? My idea would be to reference you as the author of the patch in Bug 939729 that I'm applying to upstream. Beyond that, I'm not sure. :)
Flags: needinfo?(felash)
Applied the changes from bug 939729 by Julien Wajsberg [:julienw]
to addon-sdk/source/test/addons/e10s-content/lib/httpd.js.
Removed whitespaces.
Attachment #8882786 - Flags: review?(ahalberstadt)
Attachment #8882547 - Attachment is obsolete: true
Comment on attachment 8882786 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file  ahal

I hope this reference in the commit log suits Julien :)
Requesting review from Andrew since Joel's on holiday.
Hey Leni, looks like this latest patch still has the trailing whitespaces that Joel mentioned previously. Most editors have some way of automatically removing them (or making them visible), you could try googling how to set that up in your editor. You could also just remove them manually, but then you might miss them again in the future :).
Flags: needinfo?(lenikmutungi)
Attachment #8882786 - Flags: review?(ahalberstadt)
Thanks, this works for me :)
Flags: needinfo?(felash)
(In reply to Andrew Halberstadt [:ahal] from comment #13)
> Hey Leni, looks like this latest patch still has the trailing whitespaces
> that Joel mentioned previously. Most editors have some way of automatically
> removing them (or making them visible), you could try googling how to set
> that up in your editor. You could also just remove them manually, but then
> you might miss them again in the future :).

Hmmm... Interesting. I ran ./mach eslint against the file and it didn't show any of them. 
Will do and see.
Flags: needinfo?(lenikmutungi)
Ah, the eslint linter isn't configured to run on every file, it's likely httpd.js is still excluded. You can see the trailing whitespace in the splinter review as the red bars. Thanks!
(In reply to Andrew Halberstadt [:ahal] from comment #16)
> Ah, the eslint linter isn't configured to run on every file, it's likely
> httpd.js is still excluded. You can see the trailing whitespace in the
> splinter review as the red bars. Thanks!

Where would I find this splinter review? Apologies in advance if I'm asking something that should 
be obvious.
Flags: needinfo?(ahalberstadt)
No worries! If you looks at your attachment on this bug, you can click the "splinter review" link to see an enhanced diff. Then click on the file name and you'll see the whitespace highlighted in red.
Flags: needinfo?(ahalberstadt)
Applied the changes from bug 939729 by Julien Wajsberg [:julienw]
to addon-sdk/source/test/addons/e10s-content/lib/httpd.js.
Removed whitespaces.
Attachment #8885852 - Flags: review?(ahalberstadt)
Attachment #8882786 - Attachment is obsolete: true
Comment on attachment 8885852 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file  ahal

The whitespaces were highlighted in Nano in very much the same way as in the splinter review. I hope I've gotten rid of them.
The other files I'm planning to make this modification to are as follows:

mozilla-central/addon-sdk/source/lib/sdk/test/httpd.js
mozilla-central/addon-sdk/source/test/addons/content-permissions/httpd.js
mozilla-central/addon-sdk/source/test/addons/content-script-messages-latency/httpd.js
mozilla-central/addon-sdk/source/test/addons/e10s-content/lib/httpd.js
mozilla-central/addon-sdk/source/test/addons/places/lib/httpd.js
mozilla-central/addon-sdk/source/test/lib/httpd.js
mozilla-central/addon-sdk/source/test/test-httpd.js
mozilla-central/netwerk/test/httpserver/httpd.js
mozilla-central/services/sync/tps/extensions/mozmill/resource/stdlib/httpd.js

If there are any I shouldn't be modifying, please let me know :)
Flags: needinfo?(ahalberstadt)
(In reply to Leni Kadali from comment #21)
> The other files I'm planning to make this modification to are as follows:
> 
> mozilla-central/addon-sdk/source/lib/sdk/test/httpd.js
> mozilla-central/addon-sdk/source/test/addons/content-permissions/httpd.js
> mozilla-central/addon-sdk/source/test/addons/content-script-messages-latency/
> httpd.js
> mozilla-central/addon-sdk/source/test/addons/e10s-content/lib/httpd.js
> mozilla-central/addon-sdk/source/test/addons/places/lib/httpd.js
> mozilla-central/addon-sdk/source/test/lib/httpd.js
> mozilla-central/addon-sdk/source/test/test-httpd.js
> mozilla-central/netwerk/test/httpserver/httpd.js
> mozilla-central/services/sync/tps/extensions/mozmill/resource/stdlib/httpd.js
> 
> If there are any I shouldn't be modifying, please let me know :)

These are the files I don't think I'll be able to modify since their copies no longer exist on GitHub as far as I can tell:

addon-sdk/source/lib/sdk/test/httpd.js:
throw new Error(`This file was removed. A copy can be obtained from:
  https://github.com/mozilla/addon-sdk/blob/master/test/lib/httpd.js`);

addon-sdk/source/test/addons/content-permissions/httpd.js
addon-sdk/source/test/addons/content-script-messages-latency/httpd.js
addon-sdk/source/test/addons/places/lib/httpd.js
/*
*  NOTE: do not edit this file, this is copied from:
*  https://github.com/mozilla/addon-sdk/blob/master/test/lib/httpd.js
*/

I don't think addon-sdk/source/test/test-httpd.js is to be modified judging by its contents.
Wow, I knew we had more than one copy of httpd.js in the tree, but didn't know there were that many!

The main copy of httpd.js is netwerk/test/httpserver/httpd.js, so that one for sure needs to be modified. I'm not sure if all the others are supposed to be updated or not. Joel, do you know? I think you are right about addon-sdk/source/lib/sdk/test/httpd.js, let's not touch that one.
Flags: needinfo?(ahalberstadt)
Comment on attachment 8885852 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file  ahal

Review of attachment 8885852 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good! Could you also modify the other instances of httpd.js in the same patch please? The most important one is the one under /netwerk

::: addon-sdk/source/test/addons/e10s-content/lib/httpd.js
@@ +2653,5 @@
>        }
>      }
>      else
>      {
> +        var lastModified;

nit: please make this the same indent level as below.

@@ +2661,5 @@
> +          response.setHeader("Last-Modified", lastModified, false);
> +      }
> +      catch (e) {
> +          /* lastModifiedTime threw, ignore */
> +          lastModified = null;

httpd.js uses two space indents, please convert this patch to two spaces as well.
Attachment #8885852 - Flags: review?(ahalberstadt) → feedback+
(In reply to Andrew Halberstadt [:ahal] from comment #23)
> The main copy of httpd.js is netwerk/test/httpserver/httpd.js, so that one
> for sure needs to be modified. I'm not sure if all the others are supposed
> to be updated or not. Joel, do you know? I think you are right about
> addon-sdk/source/lib/sdk/test/httpd.js, let's not touch that one.
Flags: needinfo?(jmaher)
that list of files looks accurate, but why do we have so many copies.  I would edit them all, then file a followup bug to use the build system to copy a master copy of httpd.js to all of the places it is needed during the build phase.
Flags: needinfo?(jmaher)
Applied the changes from bug 939729 by Julien Wajsberg [:julienw]
to addon-sdk/source/test/addons/e10s-content/lib/httpd.js.
Removed whitespaces.
Attachment #8886265 - Flags: review?(ahalberstadt)
Attachment #8885852 - Attachment is obsolete: true
Comment on attachment 8886265 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file  ahal

Hopefully this isn't too much to review all at once. :-)
Comment on attachment 8886265 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file  ahal

Review of attachment 8886265 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks leni! This looks good. I'm going to be PTO tomorrow, but if you need help landing this maybe jmaher can help. Otherwise I'll be back next week and can help you then.
Attachment #8886265 - Flags: review?(ahalberstadt) → review+
(In reply to Andrew Halberstadt [:ahal] from comment #29)
> Comment on attachment 8886265 [details] [diff] [review]
> With httpd.js we sometimes don't get the most recent version of the file 
> ahal
> 
> Review of attachment 8886265 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks leni! This looks good. I'm going to be PTO tomorrow, but if you need
> help landing this maybe jmaher can help. Otherwise I'll be back next week
> and can help you then.

My apologies, but I have never landed a patch myself. If you don't mind, could you guide me through how to do it?
Flags: needinfo?(ahalberstadt)
Sorry Leni, I forgot to do this. You won't be able to land without special commit access to the repository. The first step is to test this patch out on try server. If the results look good then I can land it for you, and if not I'll let you know. I'll leave the needinfo here to remind myself to do this.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e96c2ecd3a47ba60c83b7b27c1f98685a6243f5a
Assignee: nobody → lenikmutungi
Status: NEW → ASSIGNED
Flags: needinfo?(ahalberstadt)
Flags: needinfo?(ahalberstadt)
Uh oh, looks like there are a lot of test failures in the try push.

Leni, this isn't your fault or anything, your patch is correct. This is happening because we have *a lot* of tests using httpd.js, and even a slight change will often cause a bunch of them to fail. I don't think hunting down and fixing all the failing tests should fall on you, and I suspect this wasn't actually a good first bug after all :(

I think the best thing for now is to leave this bug alone, you've done your part, and hopefully we can figure out how to land this without breaking any tests at some point in the near future. I'm really sorry about that. But if you want to find something else to work on in the meantime, I'd be happy to help you find something.

Joel, I'm removing the good first bug tag, how do you want to proceed here?
Flags: needinfo?(ahalberstadt) → needinfo?(jmaher)
Whiteboard: [good first bug][lang=js] → [lang=js]
I think this bug should be tackled when 57 is off of nightly (or even when it is in release).  We are changing a lot of test scheduling and frameworks now, possibly some of these will be easier to copy via the build system, or at the very least we will be running fewer tests and the failures will be easier to understand.
Flags: needinfo?(jmaher)
(In reply to Andrew Halberstadt [:ahal] from comment #32)
> Uh oh, looks like there are a lot of test failures in the try push.
> 
> Leni, this isn't your fault or anything, your patch is correct. This is
> happening because we have *a lot* of tests using httpd.js, and even a slight
> change will often cause a bunch of them to fail. I don't think hunting down
> and fixing all the failing tests should fall on you, and I suspect this
> wasn't actually a good first bug after all :(
> 
> I think the best thing for now is to leave this bug alone, you've done your
> part, and hopefully we can figure out how to land this without breaking any
> tests at some point in the near future. I'm really sorry about that. But if
> you want to find something else to work on in the meantime, I'd be happy to
> help you find something.
No worries. I think I have all the JS good first bugs bookmarked so finding what to do next shouldn't be a problem :-)
Component: httpd.js → General
You need to log in before you can comment on or make changes to this bug.