Maintenance: refactor tests (or implementation) so that we can remove usage of literal keys in some tests

[daschaa]

Summary

In the test files we have a lot of accesses that look like object['property'. By default, biome throws an error if this kind of access is used and it suggests to use the literal key access instead (object.property). However, in our test cases this can not be done, because the properties are private and therefore this kind of access would fail.

Therefore we need to extend the biome configuration to override the linting for test files, to ignore this rule in test files. I suggest to add the following property to the biome.json

{
"overrides": [
    {
      "include": ["**/*.test.ts"],
      "linter": {
        "rules": {
          "complexity": {
            "useLiteralKeys": "off"
          }
        }
      }
    }
  ],
}

Why is this needed?

We do not want to flood our test files with biome exception comments.

Which area does this relate to?

Tests

Solution

No response

Acknowledgment

• [X] This request meets Powertools for AWS Lambda (TypeScript) Tenets
• [ ] Should this be considered in other Powertools for AWS Lambda languages? i.e. Python, Java, and .NET

Future readers

Please react with 👍 and your use case to help us understand customer demand.

[daschaa]

@dreamorosi If you agree with the approach, I would like to take the issue :)

[dreamorosi]

Hey @daschaa - thank you for opening the issue and proposing a solution.

Before moving forward I'd like us to spend some time evaluating if there's any other option that would allow us to make these properties more accessible.

At least for some of them, namely the console, I know some customers might want to access it in their own tests, so perhaps converting them to protected could be a good idea (I don't know yet!) as it would allow us to do something like this in the tests:

class MockMetrics extends Metrics {
  public declare console: Console;
}

// now you can spy on console

it('does stuff', () => {
  const metrics = new MockMetrics();
  const logSpy = vi.spyOn(metrics.console, 'log'); // <- this works
})

Alternatively, maybe this is a signal that we are testing the code in the wrong way and we should instead refactor the tests to observe side effects rather than reach into the internals of the class.

For example, let's take this test as example:

test('it should take consideration of defaultDimensions while throwing error if number of dimensions exceeds the maximum allowed', () => {
    // Prepare
    const defaultDimensions: LooseObject = {
      environment: 'dev',
      foo: 'bar',
    };
    const metrics: Metrics = new Metrics({
      namespace: TEST_NAMESPACE,
      defaultDimensions,
    });
    const dimensionName = 'test-dimension';
    const dimensionValue = 'test-value';

    // Act & Assess
    // Starts from 3 because three default dimensions are already set (service, environment, foo)
    expect(() => {
      for (let i = 3; i < MAX_DIMENSION_COUNT; i++) {
        metrics.addDimension(
          `${dimensionName}-${I}`,
          `${dimensionValue}-${I}`
        );
      }
    }).not.toThrowError();
    // biome-ignore  lint/complexity/useLiteralKeys: This needs to be accessed with literal key for testing
    expect(Object.keys(metrics['defaultDimensions']).length).toBe(3);
    // biome-ignore  lint/complexity/useLiteralKeys: This needs to be accessed with literal key for testing
    expect(Object.keys(metrics['dimensions']).length).toBe(
      MAX_DIMENSION_COUNT - 3
    );
    expect(() =>
      metrics.addDimension('another-dimension', 'another-dimension-value')
    ).toThrowError(
      `The number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}`
    );
  });
});

Here we are reaching into private properties to check that the dimension count doesn't exceed the desired value. Assuming we were able to capture the log output, we could instead flush the buffer and count how many dimensions were added to the output.

--

Ti sum up, I don't know if any of the other options is viable and/or how much effort it'd take, however before setting the ignore and moving on I'd like to see if there's any opportunity to avoid the suppression and instead raise the bar. Usually these rules are there for a reason, and I'm not 100% sure we are the exception to the rule.

[daschaa]

@dreamorosi Seeing it from this perspective - that we can also think about changing the implementation/tests - i definitely agree with your points. Should this be evaluated for each module/utility seperately or in one issue? 🤔

[dreamorosi]

I think the two modules mostly affected by this are Logger and Metrics, and for Logger I think the main property we're accessing is Logger.console.

I'd say maybe let's discuss them both together and then once we settle on a solution for each, we can address them in separate PRs.

[dreamorosi]

I have worked on the first half of this issue by refactoring the unit tests for Logger completely.

You can see more details in #2942, but I was able to bring down the usage of literal keys to a single occurrence.

As time allows it, we'll try to do the same for the Metrics package.

[github-actions[bot]]

This is now released under v2.12.0 version!