Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ECO-4757] Align ably-js tests with spec items #1806

Merged
merged 50 commits into from
Jul 17, 2024
Merged

[ECO-4757] Align ably-js tests with spec items #1806

merged 50 commits into from
Jul 17, 2024

Conversation

VeskeR
Copy link
Contributor

@VeskeR VeskeR commented Jun 26, 2024

This PR marks all ably-js tests in correspondence with the spec items each test covers.
This is achieved by adding docstrings for test case functions with the following tags:

  • @spec - The test case directly tests all the functionality documented in the spec item.
  • @specpartial - The test case partially tests the functionality documented in the spec item. This can be due to the spec item having conditional statements, the spec item being too overloaded and covering points that could be split into multiple spec items, or the spec item being documented for the parent class but the test case tests the functionality for one of its descendant classes.
  • @nospec - No corresponding spec item was found for the test.
  • @specskip - The test case is skipped during CI, so we should not take spec items related to this test case into account when deciding on metrics for spec coverage/feature tracking.

This information will be used in ECO-4852 (see #1811) to catalog the current state of specification coverage by ably-js tests and decide on future refactorings/improvements to the specification in ECO-12.

Additionally, docstrings may include references to other related spec items. These spec items are not tested directly by the test case but may provide additional context for the test.

@github-actions github-actions bot temporarily deployed to staging/pull/1806/features June 26, 2024 12:15 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/bundle-report June 26, 2024 12:15 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/typedoc June 26, 2024 12:15 Inactive
@VeskeR VeskeR marked this pull request as draft June 26, 2024 12:20
@github-actions github-actions bot temporarily deployed to staging/pull/1806/features July 1, 2024 07:48 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/bundle-report July 1, 2024 07:49 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/typedoc July 1, 2024 07:49 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/features July 1, 2024 07:52 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/typedoc July 1, 2024 07:53 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/bundle-report July 1, 2024 07:53 Inactive
@VeskeR VeskeR marked this pull request as ready for review July 1, 2024 08:20
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a heroic effort, well done! I haven't looked at all of the annotations in detail, but here are a few thoughts on the approach:

  • It would probably be good to add your above description of the various tags as documentation somewhere in the codebase, along with something saying that these tags should be added for future tests.
  • Is there a way of knowing whether a given spec item is currently fully tested or not? The presence of an @spec for a given spec item indicates that it is fully tested. But what about in the case where an item is fully tested but across multiple tests? In that case there will be multiple @specpartial for that spec item, but it's not clear whether there's a way of knowing whether the spec item is fully tested or not.
  • I wonder whether there's any value in marking the React Hooks tests? Given that it's a completely separate product to the SDK, we already know that doesn't implement any of the spec points.

@VeskeR
Copy link
Contributor Author

VeskeR commented Jul 3, 2024

  • It would probably be good to add your above description of the various tags as documentation somewhere in the codebase, along with something saying that these tags should be added for future tests.

Good idea! Added it to CONTRIBUTING.md for now, you can check the wording there.

  • Is there a way of knowing whether a given spec item is currently fully tested or not? The presence of an @spec for a given spec item indicates that it is fully tested. But what about in the case where an item is fully tested but across multiple tests? In that case there will be multiple @specpartial for that spec item, but it's not clear whether there's a way of knowing whether the spec item is fully tested or not.

Unfortunately, no. I was encountering situations like you described, where a spec item was technically fully covered, but only by a set of tests. I couldn't figure out a convenient and non-cumbersome way to mark such tests so it would be possible to deduce that the spec item is fully covered by the combination of those tests. I decided to leave it as @specpartial for now, and I believe it's better to refactor the spec items themselves. If we need a couple of different tests to cover a spec item, usually it's a sign that we can split the spec item into more granular ones. This will be part of ECO-12.

  • I wonder whether there's any value in marking the React Hooks tests? Given that it's a completely separate product to the SDK, we already know that doesn't implement any of the spec points.

Consistency + I wanted to be explicit about which spec items each test is covering (if any) and leave no room for interpretation. If we don't mark those tests, then a new person may wonder why they are not marked and if the spec for them actually exists somewhere. On the other hand, @nospec immediately indicates that there is simply no spec for them.

@github-actions github-actions bot temporarily deployed to staging/pull/1806/features July 4, 2024 10:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/typedoc July 4, 2024 10:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/bundle-report July 4, 2024 10:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/bundle-report July 4, 2024 10:39 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/features July 4, 2024 10:39 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1806/typedoc July 4, 2024 10:39 Inactive
Copy link
Collaborator

@ttypic ttypic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks amazing, I like it!

@VeskeR VeskeR merged commit 09f8162 into main Jul 17, 2024
12 checks passed
@VeskeR VeskeR deleted the uts-logs branch July 17, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants