Skip to content

fix: incorrect offset in fixUrl function#1184

Merged
szmarczak merged 1 commit intomasterfrom
fix-fix-url
Sep 20, 2021
Merged

fix: incorrect offset in fixUrl function#1184
szmarczak merged 1 commit intomasterfrom
fix-fix-url

Conversation

@szmarczak
Copy link
Copy Markdown
Contributor

Fixes #1183

@szmarczak szmarczak requested a review from B4nan September 20, 2021 09:43
@B4nan
Copy link
Copy Markdown
Member

B4nan commented Sep 20, 2021

cc @vladfrangu, looks like the new infinite scrolling implementation makes this test flaky. would be good to do something about that, better to have it skipped if it would be too flaky, it can easily just add noise instead of value that way

FAIL test/puppeteer_utils.test.js (7.79 s)
  ● Apify.utils.puppeteer › with launchPuppeteer › infiniteScroll() › stopScrollCallback works

    expect(received).toBe(expected) // Object.is equality

    Expected: true
    Received: false

      398 |                 // The test passes because the Infinite waitForSecs is broken by the callback.
      399 |                 // If it didn't, the test would time out.
    > 400 |                 expect(after).toBe(true);
          |                               ^
      401 |             });
      402 |         });
      403 |

      at Object.<anonymous> (test/puppeteer_utils.test.js:400:31)
          at runMicrotasks (<anonymous>)

@szmarczak szmarczak merged commit bc8c0a4 into master Sep 20, 2021
@szmarczak szmarczak deleted the fix-fix-url branch September 20, 2021 09:51
@vladfrangu
Copy link
Copy Markdown
Member

cc @vladfrangu, looks like the new infinite scrolling implementation makes this test flaky. would be good to do something about that, better to have it skipped if it would be too flaky, it can easily just add noise instead of value that way

FAIL test/puppeteer_utils.test.js (7.79 s)
  ● Apify.utils.puppeteer › with launchPuppeteer › infiniteScroll() › stopScrollCallback works

    expect(received).toBe(expected) // Object.is equality

    Expected: true
    Received: false

      398 |                 // The test passes because the Infinite waitForSecs is broken by the callback.
      399 |                 // If it didn't, the test would time out.
    > 400 |                 expect(after).toBe(true);
          |                               ^
      401 |             });
      402 |         });
      403 |

      at Object.<anonymous> (test/puppeteer_utils.test.js:400:31)
          at runMicrotasks (<anonymous>)

The issue isn't really the implementation of the scrolling, but the fact that it's not instant like window.scrollBy (to emulate a more human-like scroll), and past tweaking the delay between the scrolls.. I don't think (but do correct me if I'm wrong) there's anything more we can do.. 😢
From my testing, with stopScrollCallback to true it almost scrolls to the bottom at times (increasing the delay between scrolls to 150 made it almost perfect on my PC and in some CIs..). I guess we could increase the delays between scrolls to 250ms instead? Might be more ideal for users too I suppose

@B4nan
Copy link
Copy Markdown
Member

B4nan commented Sep 20, 2021

Yeah either increase the timeout or disable the test. I already disabled most of the flaky tests in past weeks.

@vladfrangu
Copy link
Copy Markdown
Member

Let's increase the timeout to 250ms and see what happens 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

requestAsBrowser's fixURL() sometimes breaks properly encoded URLs

3 participants