-
Notifications
You must be signed in to change notification settings - Fork 74
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
Conversation
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`, { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
40c07be
to
8278091
Compare
8278091
to
0c45461
Compare
39c403a
to
4e55b2f
Compare
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`, { |
There was a problem hiding this comment.
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.
4e55b2f
to
8425882
Compare
8425882
to
fd26a2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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
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 forendo 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:
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 acontext
object which can be passed towithContext
to make a wrapper. This wrapper can be applied to a test routineY
to replay the state-changing commands from the testX
before callingY
, then clean up the state changes fromX
afterward.The tests are all declared to ava in the same
index.test.js
file, but section tests can be selected independently withyarn test --match <section-name-1> [--match <section-name-2> [...]]
, for exampleyarn 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