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

Resolve issues found in reviewed PRs 4699, 4703, 4711 [, ...] #4731

Merged
merged 20 commits into from
Nov 2, 2020

Conversation

ghubstan
Copy link
Contributor

@ghubstan ghubstan commented Oct 30, 2020

A chain of 18 PRs are being reviewed in parent-branch -> child-branch order, and the reviewer agreed to allow me to put requested changes into its own PR branch -- this one, the final branch of the chain.

The first two commits resolve issues in #4699, #4703, #4711

When all issues in the PR chain are resolved here, this PR will move from draft to ready for review status.

The CoreTradesService was refactored to work for newly added api methods:

- keepfunds -- close trade, keep funds in bisq wallet

- withdrawfunds -- close trade, withdraw funds to external btc wallet

A getKey accessor was added to CoreWalletsService (needed by withdrawfunds impl).
Some refactoring of the api test case hierarchy is included in this commit.
This commit fixes non-trade tests broken by the last refactoring.
The implementation will be added to CoreOffersService in the next PR.
This upgrades jupiter for all subprojects using this library, but
it does not affect released code.
- Show full stack traces in console
- Do use previously cached test outputs
- Do not show skipped test name in console (too noisy)
- Show test run summary, including number of skipped tests
- Show note about skipped test info in the html report
- Show link to html report on test SUCCESS
- Remove dead code from AbstractLinuxProcess.

- Make default dummy accts static in MethodTest.

- Simplify gRPC stub creation, btc block generation, dispute agent
  registration, and dummy acct initialization in test case startup.

- Make ExpectedProtocolStatus visible to scenario test pkg.
Consolidated all method tests into fewer test cases by running them
from new 'scenario' test cases.  This cuts the current scaffold
setup & teardown times by almost 1/2.  No method tests were deleted
or duplicated.

(1)  Disabled all method (unit) test cases at the class level.
(2)  Added new scenario test cases to run all disabled test cases (1).

The method test cases can still be run by commenting out the @disabled
annotation.
These changes were requested in review of PR
bisq-network#4699
This change resolves issue found in PR review.  See
bisq-network#4703 (comment)
@ghubstan ghubstan changed the title [WIP] Resolves issues found in reviewed PRs 4699 [, ...] [WIP] Resolves issues found in reviewed PRs 4699, 4703 [, ...] Oct 30, 2020
@ghubstan ghubstan changed the title [WIP] Resolves issues found in reviewed PRs 4699, 4703 [, ...] [WIP] Resolve issues found in reviewed PRs 4699, 4703 [, ...] Oct 30, 2020
This change was requested in
chimp1984@961703e

This replaces and closes PR bisq-network#4558
@ghubstan ghubstan changed the title [WIP] Resolve issues found in reviewed PRs 4699, 4703 [, ...] [WIP] Resolve issues found in reviewed PRs 4699, 4703, 4711 [, ...] Oct 31, 2020
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

utACK

I think this is ready to merge once [WIP] is removed

if (trade == null)
return "";

Offer offer = trade.getOffer();
Copy link
Member

Choose a reason for hiding this comment

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

This is not required, but I do like the notation
Offer offer = checkNotNull(trade.getOffer());

@ghubstan ghubstan changed the title [WIP] Resolve issues found in reviewed PRs 4699, 4703, 4711 [, ...] Resolve issues found in reviewed PRs 4699, 4703, 4711 [, ...] Nov 2, 2020
@ghubstan ghubstan marked this pull request as ready for review November 2, 2020 14:06
@sqrrm sqrrm merged commit 84ac65b into bisq-network:master Nov 2, 2020
@ghubstan ghubstan deleted the 19-sqrrm-pr-changes branch November 2, 2020 15:09
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.

2 participants