Add integration test to check inter-frame communication

[esanzgar]

This new integration test checks that PortFinder, PortProvider and
PortRPC work in harmony. A change in one could lead to the others no
working as planned.

[esanzgar]

I have created three simpler tests the following three communication channels:

• guest-host
• guest-sidebar (a little bit more complex)
• sidebar-host

[codecov]

Codecov Report

Merging #4090 (dbc8c92) into master (53f0618) will not change coverage.
The diff coverage is n/a.

❗ Current head dbc8c92 differs from pull request most recent head a49c51c. Consider uploading reports for the commit a49c51c to get more accurate results

@@           Coverage Diff           @@
##           master    #4090   +/-   ##
=======================================
  Coverage   99.06%   99.06%           
=======================================
  Files         217      217           
  Lines        8170     8170           
  Branches     1918     1918           
=======================================
  Hits         8094     8094           
  Misses         76       76           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53f0618...a49c51c. Read the comment docs.

[esanzgar]

We exchanged comments about the subclassing vs using helper functions here

Please, let me know if you want me to using helper functions.

[robertknight]

I think the subclassing is OK for now. There are ways it could be abstracted, but the current implementation is easy enough to follow.

I had a suggestion regarding the grouping of the code that represents each of the different frames in the test, namely to split them into separate functions, to make it clear that they are meant to represent activities happening in different frames which run concurrently and don't share any state (except via a message channel).

Generally LGTM though.

[robertknight]

The done resolver could potentially be replaced by doing the portFinder.discover call in the Promise callback:

const done = new Promise(resolve => {
  portFinder.discover('host').then(port => {
    hostRPC.connect(port);
    hostRPC.call('ping', response => {
      assert.equal(response, 'pong');
      resolve();
    });
  });
});

...

return done;

[esanzgar]

Great idea! Thanks!

[robertknight]

Can you clarify what you mean by realism here?

[esanzgar]

Guest can be ready before the Host frame. If that's the case, Guest will polls the Host. I was trying to simulate that scenario.

[robertknight]

I suggest to remove these comments, and leave it to the PortRPC docs to explain that correct usage of the class requires calling on before connect.

[esanzgar]

👍🏼

[robertknight]

To summarize what these tests are doing, each runs a bunch of concurrent activities that simulate the guest, host and sidebar roles in the inter-frame communication process. All the only communication between frames has to go via message channels (no shared variables). One way to make this structure more obvious might be to wrap each "actor" in a function and then run all the actors concurrently at the end.

it('enables ... communication', async () => {
  const simulateGuest = async () => {
    // Guest steps here
  };

  const simulateHost = async () => {
    // Host steps here
  };

  return Promise.all([simulateGuest(), simulateHost()]);
});

[esanzgar]

Very good idea! Thanks!

[robertknight]

Suggested change

	const portFinder1 = new PortFinder({
	const guestPortFinder = new PortFinder({

Alternatively, if you wrap each actor in a separate function, the variable names won't conflict.

[esanzgar]

I use separate functions.

[robertknight]

If you are going to use this pattern, I'd suggest to let done be the promise as it is a name of a state (similar to loaded or completed), and call the resolver resolveDone. Alternatively, skip the resolver variable and just use the promise callback:

const done = new Promise(resolve => {
   ...
   hostRPC.call('ping', response => { ...; resolve(); });
});

[esanzgar]

I incorporated your feedback on the second commit.

[esanzgar]

Very good idea! Thanks!

[esanzgar]

Great idea! Thanks!

[esanzgar]

👍🏼

[esanzgar]

I use separate functions.