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

Vault test refactor #5103

Merged
merged 8 commits into from
Apr 14, 2022
Merged

Vault test refactor #5103

merged 8 commits into from
Apr 14, 2022

Conversation

dtribble
Copy link
Member

refs: #4568

Description

Refactor test support into shared module

  • add test/support.js for test helpers
  • consolidate makeBundle and setupZoeForTest
  • make setupZoeForTest sync and return promises
  • add bootstrap space setup helper

Refactor vault tests:

  • add test.before for shared setup
  • use t.context pervasively for shared and default config
  • change methods to override t.context before invoking setupServices
  • use econ-behaviors directly
  • move addVaultType into shared setup
  • eliminate governance terms

Security Considerations

N/A

Documentation Considerations

N/A

Testing Considerations

This simplifies setup for RUN tests, and dramatically reduces duplicate code and setup in vault tests especially. It also reduces some redundant work in tests like creation of a new Zoe for each independent test. Additional shared overhead is now possible via test.before

@dtribble dtribble requested a review from turadg as a code owner April 13, 2022 00:56
@dtribble dtribble requested a review from Chris-Hibbert April 13, 2022 03:58
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

The PR title omits that src is being refactored but the commits look good. I assume the PR will be merged with out squashing.

const debugTick = (...args) => {
console.log(key, (debugCount += 1), ...args);
const debugTick = (optLog, ...args) => {
if (optLog.log) {
Copy link
Member

Choose a reason for hiding this comment

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

I hope we find time after MN-1 to rationalize logging. I don't think we have a ticket yet for that per se. #1318 is to do research.

@@ -157,6 +157,7 @@ export const makeVaultManager = (
liquidationInProgress = false;
// XXX should notify interested parties
console.error('liquidateAndRemove failed with', e);
throw e;
Copy link
Member

Choose a reason for hiding this comment

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

non-blocking: this is out of scope.

@@ -68,13 +68,14 @@ const liquidate = async (
collateralBrand,
);

// XXX problems with upgrade
Copy link
Member

Choose a reason for hiding this comment

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

gratuitous removal. this is still true.

@dtribble dtribble added the automerge:no-update (expert!) Automatically merge without updates label Apr 13, 2022
@dtribble dtribble removed the automerge:no-update (expert!) Automatically merge without updates label Apr 14, 2022
@dtribble dtribble requested a review from Chris-Hibbert April 14, 2022 04:38
@dtribble
Copy link
Member Author

@Chris-Hibbert I had included the new manualPriceAuthority and test changes with some cleanups here. Rather than separate them out, since they are really part of this overall set of changes, can you review the last commit and provide additional approval (if appropriate)?

dtribble and others added 6 commits April 13, 2022 21:50
* turn off unneeded tracing
* pass `debt` to liquidation as an offerArg
* add trace info in liquidation
* add test/support.js for test helpers
* consolidate `makeBundle` and `setupZoeForTest`
* make `setupZoeForTest` sync and return promises
* add bootstrap `space` setup helper

Refactor vault tests:
* add `test.before` for shared setup
* use `t.context` pervasively for shared and default config
* change methods to override `t.context` before invoking `setupServices`
* use `econ-behaviors` directly
* move `addVaultType` into shared setup
* eliminate governance terms
* deleted accidental pre-release stuff
* added additional type declarations
* introduce `manualPriceAuthority` for tests
* use manualPriceAuthority for example test
* further DRY test code
@dtribble dtribble force-pushed the vault_test_refactor branch from 18eebf4 to 744e524 Compare April 14, 2022 04:53
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

These are improvements.


priceAuthority.setPrice(makeRatio(677n, runBrand, 900n, aethBrand));
trace(t, 'price dropped a little');
// await manualTimer.tick();
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to leave these tick()s here long term?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope. I'll clean that up in a future PR.

@dtribble dtribble added the automerge:no-update (expert!) Automatically merge without updates label Apr 14, 2022
@mergify mergify bot merged commit 4e1e3bf into master Apr 14, 2022
@mergify mergify bot deleted the vault_test_refactor branch April 14, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants