-
Notifications
You must be signed in to change notification settings - Fork 240
Conversation
214c479
to
67024a2
Compare
f2a2930
to
2757635
Compare
2757635
to
2314488
Compare
test/framework/assertions.ts
Outdated
value: any; | ||
}, | ||
): Promise<void> { | ||
expect(actual[input.field]).to.be.deep.eq(input.value); |
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.
nice!
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.
Will expect
log a helpful error message if it fails?
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.
As of now, no. I'll add that as an optional parameter to the AssertionInfo
interface. I'm going to improve error handling today, which should mitigate this to an extent.
test/framework/test_manager.ts
Outdated
|
||
export type AssertionType = string; | ||
|
||
export interface ActionInfo { |
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.
Maybe just Action
and Assertion
?
Action {
name: ActionName,
input: ...
}
Assertion {
name: AssertionName,
input: ...
}
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 like the name
suggestion. I'll definitely add that.
As far as the ActionInfo
=> Action
and AssertionInfo
=> Assertion
changes, I'm not completely sold yet. I originally used this naming convention, but test cases really just describe the actions and assertions that must be used rather than "being" the actions and assertions. Furthermore, I think that Action
and Assertion
better describe the types which I ultimately named Action
and Assertion
. What are your thoughts on this?
return new Promise<void>(resolve => { | ||
setTimeout(resolve, duration); | ||
}); | ||
} |
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.
deployment is pretty sweet 🙌 any idea how much time processes take to start up/tear down this way?
One thing I see needing is a way to delete all orders from the postgres cache between each test. We don't necessarily need to wipe all the deps, just clear the saved orders. Would this be an appropriate place for a function like that?
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.
The sleeps provide a hard cap on how long it takes. As per Fabio's comment below, I am now deleting the database and mesh records, which added several seconds. It's looking like around ~15 - 20s for setup and very few (1 - 4s at most) for teardown.
I haven't really worked on this at all since I'm just trying to get the framework out the door so we can start to write some tests, but I will look into scraping the logs this afternoon.
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 is looking pretty sweet so far!
I'm still trying to think of how to chain test cases or actions. We definitely need a way to run a series of actions - looking at the current implementation, this seems straightforward.
We probably need a way to run a series of actions with interspersed assertions, which looks like it will be a bit more complicated.
I started thinking about something like this:
type TestCaseItem = Action | Assertion
type TestCase = TestCaseItem[] // all items share the same state within the test case
type TestSuite = TestCase[] // cases in a suite test a similar responsibility, but don't implicitly share state
ActionX<A,B> (input: A): B
AssertionJ<B>(input: Partial<B>): B
ActionY<B, C> (input: B): C
AssertionK<C> (input: Partial<C>): C
...
Basically passing the inputs and outputs through the Action
and Assertion
containers.
But this would lead to having to make an exhaustive list of TestCaseItem
types, which is a bit clunky.
Any thoughts on how to implement this?
test/app_test.ts
Outdated
{ | ||
description: 'should respond to GET /sra/orders', | ||
action: { | ||
actionType: 'apiGetRequestAsync', |
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.
Could we make this an enum?
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.
We can, but that will really reduce the flexibility of the framework (this might be a good thing, but I'm not convinced of that right now). Something that might be a good middle ground is to create constants for common actions and assertions. I'll add this in the next commit.
test/framework/assertions.ts
Outdated
value: any; | ||
}, | ||
): Promise<void> { | ||
expect(actual[input.field]).to.be.deep.eq(input.value); |
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.
Will expect
log a helpful error message if it fails?
* @param shouldPrintDependencyLogs Whether or not the 0x-api's dependencies | ||
* should surface their logs. | ||
* TODO(jalextowle): It would be a good improvement to be able to specify log | ||
* files where the logs should actually be written. |
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.
👍
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.
As of now, does it output the logs to the same stdout as the test results?
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.
Yep. Until the log output can be redirected to a file, I've been seeing this as a feature that developers will use while writing and debugging tests, but not as much afterwards.
test/utils/deployment.ts
Outdated
// Wait for the dependencies to boot up. | ||
// HACK(jalextowle): This should really be replaced by log-scraping, but it | ||
// does appear to work for now. | ||
await sleepAsync(10000); // tslint:disable-line:custom-no-magic-numbers |
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.
Yikes, does it take that look to boot up? Would log scraping make this faster?
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.
That's the upper limit (mostly because of docker pull
's inconsistent timing on different runs. I think log scraping will make this considerably faster most of the time.
Overview
There are several problems with the API's current testing infrastructure. This PR intends to solve all of them by creating a testing framework that melds many of the earlier solutions together into a cohesive piece of infrastructure that can be maintained by the 0x-api team.