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

test(cli): Add demo doc alignment test, serial implementation #2384

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

grypez
Copy link
Contributor

@grypez grypez commented Jul 26, 2024

Closes: #2368
Refs: #2372

Description

A test suite which asserts that the cli demo produces its documented outputs.

Current weaknesses in the implementation include concurrency and independence. They are related and both should be resolved by a change to the daemon initialization in the test setups.

Note that despite these weaknesses, this setup is useful enough to reveal that certain cli commands do not behave as the demo supposes. Notably:

  • endo list --as alice-agent is rejected because --as is not a supported option for endo list. This could have been caught with unit tests on the cli endpoints.
  • endo inbox does not display messages as expected. In some cases the template string for the message has simply been altered, but the discrepancy in the mailboxes-are-symmetric section is unlikely to have been caught with a unit test.

Testing Considerations

The familiar-chat section is not tested.
Three of the section tests are failing, which is correct test behavior.

Concurrency

Ideally each tested section would run concurrently, however as written the demo sections are implicitly serial. This is a nice UX for a demo and I don't suggest changing it.

Thus tests running on the same daemon which reference the same objects interfere with one another. Possible solutions:

  1. Run a different endo daemon per test, configured to its own directory
  2. Test a slightly modified version of each section which prefixes petnames with a per-test unique id
  3. Run the tests serially
  4. Some other solution of which I am unaware

In this PR I have opted for (3.) test the sections serially. The logic to test each section is written in a distinct file bearing the name of that section, but the routines are imported into a common index.test.js and declared to ava as serial.

In a future PR it may be nice to adopt (1.), reusing setup from daemon/test/endo.test.js. I think prepareConfig accomplishes what I have in mind.

Independence

Although the tests are run serially, each depended-upon test file X exports setup and teardown routines in a context object which can be passed to withContext to make a wrapper. This wrapper can be applied to a test routine Y to replay the state-changing commands from the test X before calling Y, then clean up the state changes from X afterward.

The tests are all declared to ava in the same index.test.js file, but section tests can be selected independently with yarn test --match <section-name-1> [--match <section-name-2> [...]], for example yarn test --match doubler-agent --match messages-are-symmetric. The independence is weak, as any breakage in the state changing commands in the previous sections will cause the setup for subsequent tests to fail. Stronger test independence is ideal, but probably a long way away.

Upgrade Considerations

  • Adds the execa dev dependency.

@grypez grypez requested a review from rekmarks July 26, 2024 22:14
packages/cli/test/demo/names-in-transit.js Outdated Show resolved Hide resolved
packages/cli/test/daemon-context.js Outdated Show resolved Hide resolved
packages/cli/test/daemon-context.js Outdated Show resolved Hide resolved
packages/cli/test/demo/index.test.js Show resolved Hide resolved
stdout: /^1\. "HOST" sent "Please enjoy this @counter\."/,
});
await testLine(execa`endo adopt --as alice-agent 1 counter --name redoubler`);
await testLine(execa`endo list --as alice-agent`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine this is the first thing that fails in this test. There's an asymmetry in the API here: rather than endo list --as <agent>, you simply do endo list <agent>. Does that change cause these test cases to succeed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think your suggestion does what you think it does, but I want to limit the scope of this PR to correctly implementing a test of whether the expectations set in the demo's README.md match the observations from running the demo's commands. A future PR could make these tests pass either by changing the behavior of endo/cli or by altering the demo's expectations. In the former case it will be satisfying to associate the fixing change to the removal of failing in these tests. In the latter case it will be nice to have an example PR conjoining edits to the README.md with edits to these tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, although in this case it's the README that's wrong; --as was omitted from list in a deliberate design decision. I noticed the discrepancy myself a while back and even put up a PR for it (#2228), but we decided against making the change at the time. I don't think the situation is likely to change unless we complete a more thorough audit of each command for --as support, and until then I think it makes the most sense to update the readme and these tests.

Copy link
Contributor

@rekmarks rekmarks Aug 13, 2024

Choose a reason for hiding this comment

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

(This comment is part of a thread, but may appear detached from it.) I don't think we actually resolved this point, but I'll let it go in exchange for you resurrecting #2228

Edit: I see you created #2409. This should be all set then. We ought to make these tests pass in order to close that issue. I also think we can resurrect #2228 if want to do so.

@grypez grypez force-pushed the test-cli-demo-serial branch 2 times, most recently from 40c07be to 8278091 Compare August 1, 2024 05:09
@grypez grypez requested a review from rekmarks August 13, 2024 03:05
@grypez grypez force-pushed the test-cli-demo-serial branch 2 times, most recently from 39c403a to 4e55b2f Compare August 13, 2024 19:30
packages/cli/test/with-context.js Outdated Show resolved Hide resolved
packages/cli/test/demo/sending-messages.js Outdated Show resolved Hide resolved
stdout: /^1\. "HOST" sent "Please enjoy this @counter\."/,
});
await testLine(execa`endo adopt --as alice-agent 1 counter --name redoubler`);
await testLine(execa`endo list --as alice-agent`, {
Copy link
Contributor

@rekmarks rekmarks Aug 13, 2024

Choose a reason for hiding this comment

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

(This comment is part of a thread, but may appear detached from it.) I don't think we actually resolved this point, but I'll let it go in exchange for you resurrecting #2228

Edit: I see you created #2409. This should be all set then. We ought to make these tests pass in order to close that issue. I also think we can resurrect #2228 if want to do so.

Copy link
Contributor

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

@grypez grypez merged commit bec206b into endojs:master Aug 14, 2024
17 checks passed
sirtimid added a commit that referenced this pull request Sep 20, 2024
Closes: #2409
Refs: #2384

## Description

This PR updates the readme to properly document the `list` cli command
and fixes the failing tests


### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

none
### Testing Considerations

none
### Compatibility Considerations

none
### Upgrade Considerations

none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test cover cli demo
2 participants