10.7.0 broke the Yarn tests #21907

Activity

1. ChALkeR

   added

   httpIssues and PRs related to the http subsystem.Issues and PRs related to the http subsystem.

   on Jul 20, 2018
2. ChALkeR commented on Jul 20, 2018

   

   ChALkeR

   on Jul 20, 2018

   Last edited by ChALkeR

   Member

   More actions

   Thas commit comes from #21204.

   /cc @Trott @nodejs/http

   @arcanis a shorter testcase would be helpful.
3. ChALkeR commented on Jul 20, 2018

   

   ChALkeR

   on Jul 20, 2018

   Last edited by ChALkeR

   Member

   More actions

   @arcanis This does not look to be observed when run without the mock from https://github.com/yarnpkg/yarn/blob/master/__tests__/__mocks__/request.js.
4. ChALkeR commented on Jul 20, 2018

   

   ChALkeR

   on Jul 20, 2018

   Last edited by ChALkeR

   Member

   More actions

   A reproducable testcase without all the other yarn stuff (but with request)

   const request = require('request');
   const https = require('https');
   const fs = require('fs');
   
   const opt = {"url":"https://registry.yarnpkg.com/yeoman-environment","method":"GET","headers":{"User-Agent":"yarn/1.10.0-0 npm/? node/v10.7.0 linux x64","Accept":"application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*"},"json":true,"gzip":true,"forever":true,"retryAttempts":0,"strictSSL":true,"cert":"","key":"","timeout":30000};
   
   const req = request(opt, () => {});
   
   req.httpModule = {
     request: function request(options, callback) {
       const loc = 'yeoman-environment.bin';
       options.agent = null;
       options.socketPath = null;
       options.createConnection = () => {
         return fs.createReadStream(loc);
       };
       return https.request(options, callback);
     }
   };

   This is basically what Yarn does.
5. arcanis commented on Jul 20, 2018

   

   arcanis

   on Jul 20, 2018

   Last edited by arcanis

   ContributorAuthor

   More actions

   You're right, seems like this is caused in particular by the mock of createConnection:

   options.createConnection = (): ReadStream => {
       return fs.createReadStream(loc);
   };

   The previous code was attaching on the connect event from the fake socket, which was never triggered and so the setTimeout was never called. Now that setTimeout is always called when the socket is there, the bug triggers.

   I think this is on Yarn, so I'm going to close this issue. Feel free to reopen if you think it hides a more problematic regression.
6. arcanis

   closed this as completedon Jul 20, 2018
7. arcanis commented on Jul 20, 2018

   

   arcanis

   on Jul 20, 2018

   ContributorAuthor

   More actions

   And thanks for your help, greatly appreciated!
8. dougwilson commented on Jul 20, 2018

   

   dougwilson

   on Jul 20, 2018

   Member

   More actions

   If it's the mock, my guess is that it is due to

     options.createConnection = (): ReadStream => {
       return fs.createReadStream(loc);
     };
   
9. dougwilson commented on Jul 20, 2018

   

   dougwilson

   on Jul 20, 2018

   Member

   More actions

   Wow a lot of conversation happened while I was posted that :) disregard
10. ChALkeR commented on Jul 20, 2018

    

    ChALkeR

    on Jul 20, 2018

    Last edited by ChALkeR

    Member

    More actions

    @arcanis This is one of the reasons why trying to keep the reproducable testcases minimal matters.

    I attempted to construct a testcase without yarn. To trace that, I noticed that the error was coming from the request module (as observed in the original trace), and tried to check how exactly are you using request. That was done by adding these two lines to it:

    --- node_modules/request/request.js
    ***************
    *** 93,94 ****
    --- 93,96 ----
      function Request (options) {
    +   console.log('Request', JSON.stringify(options));
    +   console.log((new Error()).stack)
        // if given the method property in options, set property explicitMethod to true

    That brought me directly to the mock file (and gave the short testcase in the comment above).

    Hope that helps 😉.
11. arcanis commented on Jul 20, 2018

    

    arcanis

    on Jul 20, 2018

    ContributorAuthor

    More actions

    Yeah, I actually logged the options to try to make repros, but completely forgot we mocked request, so didn't check the stack 😐
12. Trott commented on Jul 20, 2018

    

    Trott

    on Jul 20, 2018

    Member

    More actions

    Related: There's a PR to get yarn into citgm but it stalled. If someone wants to take it over and get it across the finish line, it would be great to have citgm flag these kinds of things for us before a release: nodejs/citgm#560

    👍React with 👍4Reacted by Maël Nison, Sagir Khan, Nikita Skovoroda and killa
13. targos commented on Jul 20, 2018

    

    targos

    on Jul 20, 2018

    Member

    More actions

    @ChALkeR little debugging tip: instead of the double console.log, you can do console.trace('Request', JSON.stringify(options))

    👍React with 👍2Reacted by Nikita Skovoroda and Marvin Hagemeister