refactor(server): simplify shared metrics tests

[sbruens]

Some cleanup I wanted to separate from another PR I'm working on where I'm editing these tests. I wanted to understand the test cases better to make edits safer.

This refactor reduces duplication, separates each distinct test scenario into its own test case, and clearly separates the arrange, act, and assert blocks of each test.

[fortuna]

Prefer a fake implementation (as we had before) than interaction verification: go/choose-test-double

go/choose-test-double#prefer-fakes

[sbruens]

While the "prefer fakes" advice is generally sound, I think it's not the best fit here. The current "fake" is more like a hand-rolled mock used to check interactions, as seen with the added public collectedServerUsageReport and collectedFeatureMetricsReport properties. Since the metrics collector functions are all state-changing (go/mocking#interaction-state-changes), a proper interaction verification would be clearer than a mock pretending to be a fake.

This isn't a blocker so I'll revert for this PR, but it is important to pick the right double for testing. Given how the MetricsCollectorClient interface is currently designed, a fake might not be ideal.

[fortuna]

The shared state prevents tests from running in parallel. Is this helping much?

I'd much rather see a set up call before each test, since that is more readable and makes the dependency clear. The hidden dependency hurts readability.

[sbruens]

Using beforeEach ensures a fresh state for each test case, as it's called before each test. I admit that it would be nice to have something with systematic guarantees like go/cleanstate so we don't depend on devs not introducing bugs in beforeEach and afterEach, but that's for another PR.

A setup helper method would have the same end result as beforeEach, but would mean we lose the ability to leverage Jasmine's describe() hierarchy to perform setup at the appropriate suite level. It also means we add bloat to test methods that is unnecessary, where we could simply leverage beforeEach to setup and define implicit defaults, as suggested by go/unit-testing-practices&polyglot=typescript#setUp.

[sbruens]

PS: On the cleanstate alternative, we could do something like assigning properties to each test case's this object:

interface ThisContext {
  ...
  publisher: SharedMetricsPublisher;
}

describe('OutlineSharedMetricsPublisher', function (this: ThisContext) {
  beforeEach(function (this: ThisContext) {
    ...
    this.publisher = new OutlineSharedMetricsPublisher(...);
  });

  it('some test....', function (this: ThisContext) {
    expect(this.publisher.isSharingEnabled()).toBeFalse();

    this.publisher.startSharing();

    expect(this.publisher.isSharingEnabled()).toBeTrue();
  });
});

However this rebinding is generally discouraged, and it's more convoluted, so I don't think we should do that. Just wanted to mention it for completeness.

[fortuna]

I think all that kind of magic makes the tests harder to understand. It's also not clear what is actually needed by the test.

The "Reads from config" is particularly mind-boggling as it has no action in it!

It's clearer if the test is self-contained, with no access to external state.
You can use helper methods if needed, and still use Jasmine's describe functionality. For example:

  describe('Enable/Disable', () => {
    it('Mirrors config', () => {
      const serverConfig = new InMemoryConfig({serverId: 'server-id'});
      const publisher = makeTestPublisher({serverConfig})

      expect(publisher.isSharingEnabled()).toBeFalse();

      publisher.startSharing();
      expect(publisher.isSharingEnabled()).toBeTrue();
      expect(serverConfig.mostRecentWrite.metricsEnabled).toBeTrue();

      publisher.stopSharing();
      expect(publisher.isSharingEnabled()).toBeFalse();
      expect(serverConfig.mostRecentWrite.metricsEnabled).toBeFalse();
    });

Note how makeTestPublisher allows for overriding fields.

It adds two lines, but all the logic is in one place. I don't need to look somewhere else to understand this test. And I don't need to worry about parallel execution.

[fortuna]

Let me know what you think. If you feel strongly about sticking to building state in beforeEach we could go with that.

[fortuna]

I think all that kind of magic makes the tests harder to understand. It's also not clear what is actually needed by the test.

The "Reads from config" is particularly mind-boggling as it has no action in it!

It's clearer if the test is self-contained, with no access to external state.
You can use helper methods if needed, and still use Jasmine's describe functionality. For example:

  describe('Enable/Disable', () => {
    it('Mirrors config', () => {
      const serverConfig = new InMemoryConfig({serverId: 'server-id'});
      const publisher = makeTestPublisher({serverConfig})

      expect(publisher.isSharingEnabled()).toBeFalse();

      publisher.startSharing();
      expect(publisher.isSharingEnabled()).toBeTrue();
      expect(serverConfig.mostRecentWrite.metricsEnabled).toBeTrue();

      publisher.stopSharing();
      expect(publisher.isSharingEnabled()).toBeFalse();
      expect(serverConfig.mostRecentWrite.metricsEnabled).toBeFalse();
    });

Note how makeTestPublisher allows for overriding fields.

It adds two lines, but all the logic is in one place. I don't need to look somewhere else to understand this test. And I don't need to worry about parallel execution.