fix: Only request vendorlist once #209

Fumler · 2018-09-06T10:50:05Z

Fixes #206

This PR sets a flag whenever you do a request for the vendorlist, so if there is several calls to loadVendorList() before the request is resolved and cache is saved, we wait for the first request to resolve in the other calls.

coveralls · 2018-09-06T10:55:10Z

Pull Request Test Coverage Report for Build 779

0 of 0 changed or added relevant lines in 0 files are covered.
32 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.7%) to 27.377%

Files with Coverage Reduction	New Missed Lines	%
dist/oil.bundle.js	32	27.38%

Totals
Change from base Build 732:	0.7%
Covered Lines:	1129
Relevant Lines:	3360

💛 - Coveralls

Fumler · 2018-09-06T13:02:41Z

Does not seem like your test coverage works properly? The coverage report basically say everything in oil.bundle.js is red, so the 27% coverage is just variables etc?

I don't know how to fix this. Maybe one of you guys know where to start? 😅

phogel · 2018-09-07T09:35:29Z

src/scripts/core/core_vendor_information.js

      resolve(cachedVendorList);
+    } else if (startedVendorlistRequest) {
+      // if a request for the vendorlist has been started, let's wait for it


Wouldn't it be simpler to just store the pending Promise in a variable named
pendingVendorlistPromise in line 41?
Return it in line 22 if it exists.
Nullify pendingVendorlistPromise when the result of fetchJsonData(iabVendorListUrl) is there and cachedVendorList is set.
The test for a timeout you could move inside the else block then.

Also a test for the timeout would be great! 👍

phogel · 2018-09-07T09:39:27Z

To be honest, I am not sure how exactly coveralls gets to this result. I'd say a decrease of "0.06%" is neglectable.

Fumler · 2018-09-07T12:25:08Z

Think I overcomplicated things a bit, the new commit makes it even simpler, I didn't think about fetchJsonData being a promise, so we can just return that.

And about the coveralls thing, it seems to me like nothing really works? In the screenshot below you can see everything is red in loadVendorlist even though you have a million tests for it that should cover those lines. So the 0.06% is just because I add a few new lines to the bundle but the new test I added is not reflected in the coverage?

phogel

Greatly improved 👍
Just ran the tests and they're all green.

Fumler · 2018-09-17T10:55:22Z

Any updates on this?

Fumler · 2018-09-27T10:18:54Z

Sorry for nagging, but we really want this released. Does anyone have time to CR and merge this or should we start using a fork?

@ltparis2018 @tbtz @phogel

marcelb · 2018-09-27T10:57:15Z

@Fumler Heya, I'm back in the team and will look into this today!

marcelb · 2018-09-27T11:26:28Z

src/scripts/core/core_vendor_information.js


 export function loadVendorList() {
  return new Promise(function (resolve) {
    if (cachedVendorList) {
      resolve(cachedVendorList);
+    } else if (pendingVendorlistPromise) {
+      return pendingVendorlistPromise;


Please add a test to check if this works correctly. I think this doesn't return function loadVendorList() but the inner anonymous function (resolve) {} unresolved instead. I would expect unexpected behavior where loadVendorList() is used. I think you want to return an unnested promise instead that behaves exactly like all the others.

I would probably refactor the whole function that the whole scope isn't inside that huge Promise return.

I'm not sure what you mean. The idea is:

On the first call to loadVendorList we return a promise, that when finished, will resolve to the vendorlist data.

On the second call to loadVendorList we also return a promise, that when finished, will resolve to the vendorlist data, as long as there is no cached version, and we have already started a promise

On the third call to loadVendorList we also return a promise, that when finished, will resolve to the vendorlist data, as long as there is no cached version, and we have already started a promise

.. etc

The second and third cases will only return the started promise if it exists.

"- On the second call to loadVendorList we also return a promise, that when finished, will resolve to the vendorlist data, as long as there is no cached version, and we have already started a promise"

Thats the one im not sure that it works as intended. There is a return statement returning the promise inside another promise (line 16). Not resolving, returning. (line 19)

But anyway, the tests are looking fine and should fail, if its actually broken. Thank you!
The change makes a lot of sense, thank you for the contribution and sorry for the long delay!

Im not entirely sure yet though and can't create a new version today anymore and will be on a 1 day vacation tomorrow. @ltparis2018 @tbtz Can you have a look as well and merge it tomorrow? If not I'll be back on monday and finish this up. Thanks!

im on it again :) I think we will be merging it soon, but we might do a small refactoring on it later.

@Fumler I did some tests on the code and I believe its still broken.

The test can't catch the errors though, because its also flawed. The tests done() function is executed before the Promises can resolve, so all the expects within this test wont run at all.
A quick fix is to move the done() in the last async method, which results in a failed test (because the last loadVendorList() never resolves).

There are two bugs within the function. One being returning the pendingVendorlistPromise instead of resolving it and the second one is the call to fetchJsonData which doesn't set the pendingVendorlistPromise variable probably. It resolves the right side first, resulting in a wrong value in the variable.

The test also assumes implicit timings of the code to work, which seems to be ok though, not raising any race conditions.

Please fix those remaining issues.

This line should rather read
resolve(pendingVendorlistPromise);

This of course results in nested Promises, which works.

marcelb · 2018-09-27T11:48:19Z

test/specs/core/core_vendor_information.spec.js

+      spyOn(CoreConfig, 'getIabVendorListUrl').and.returnValue("https://iab.vendor.list.url");
+
+      loadVendorList().then(() => {});
+      loadVendorList().then(() => {});


Please make also sure that the second loadVendorList().then() is called before the first one resolves. I'm not sure whether the second to fourth call is not just returning the cached version and this couldn't have race conditions. If you are please explain.

I updated the tests.

src/scripts/core/core_vendor_information.js

marcelb · 2018-10-01T13:15:35Z

test/specs/core/core_vendor_information.spec.js

+      });
+      expect(fetchSpy.calls.count()).toBe(1);
+
+      done();


This done() should be on line 77 instead to make the test work.

Good catch!

marcelb · 2018-10-02T08:33:58Z

Thanks! This is merged and we will create a new release as well soon.

Fumler added 2 commits September 6, 2018 12:45

fix: Only request vendorlist once

3682721

refactor: Remove console.log from test

e77dab6

phogel suggested changes Sep 7, 2018

View reviewed changes

refactor: Simplify checking for cached vendorlist

9a0693c

phogel approved these changes Sep 10, 2018

View reviewed changes

remove comments and add semicolon

9fff479

phogel requested review from ltparis2018 and tbtz September 10, 2018 09:02

marcelb suggested changes Sep 27, 2018

View reviewed changes

marcelb reviewed Sep 27, 2018

View reviewed changes

refactor: Change tests to make sure we do not have cached version

24ed51d

marcelb reviewed Oct 1, 2018

View reviewed changes

src/scripts/core/core_vendor_information.js Show resolved Hide resolved

marcelb reviewed Oct 1, 2018

View reviewed changes

fix: Resolve promise instead of return

4dc66cd

marcelb approved these changes Oct 2, 2018

View reviewed changes

marcelb merged commit 267e974 into as-ideas:master Oct 2, 2018

Fumler deleted the fix/only-request-vendorlist-once branch October 2, 2018 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Only request vendorlist once #209

fix: Only request vendorlist once #209

Fumler commented Sep 6, 2018

coveralls commented Sep 6, 2018 •

edited

Loading

Fumler commented Sep 6, 2018

phogel Sep 7, 2018

phogel Sep 7, 2018

phogel commented Sep 7, 2018

Fumler commented Sep 7, 2018

phogel left a comment

Fumler commented Sep 17, 2018

Fumler commented Sep 27, 2018

marcelb commented Sep 27, 2018

marcelb Sep 27, 2018

marcelb Sep 27, 2018 •

edited

Loading

Fumler Sep 27, 2018

marcelb Sep 27, 2018

marcelb Sep 27, 2018 •

edited

Loading

marcelb Oct 1, 2018

marcelb Oct 1, 2018

marcelb Oct 1, 2018

marcelb Sep 27, 2018 •

edited

Loading

Fumler Sep 27, 2018

marcelb Oct 1, 2018

Fumler Oct 2, 2018

marcelb commented Oct 2, 2018

fix: Only request vendorlist once #209

fix: Only request vendorlist once #209

Conversation

Fumler commented Sep 6, 2018

coveralls commented Sep 6, 2018 • edited Loading

Pull Request Test Coverage Report for Build 779

💛 - Coveralls

Fumler commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phogel commented Sep 7, 2018

Fumler commented Sep 7, 2018

phogel left a comment

Choose a reason for hiding this comment

Fumler commented Sep 17, 2018

Fumler commented Sep 27, 2018

marcelb commented Sep 27, 2018

Choose a reason for hiding this comment

marcelb Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelb Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelb Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelb commented Oct 2, 2018

coveralls commented Sep 6, 2018 •

edited

Loading

marcelb Sep 27, 2018 •

edited

Loading

marcelb Sep 27, 2018 •

edited

Loading

marcelb Sep 27, 2018 •

edited

Loading