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

create fast environment package #2919

Merged
merged 3 commits into from
Jun 13, 2019
Merged

create fast environment package #2919

merged 3 commits into from
Jun 13, 2019

Conversation

frrist
Copy link
Member

@frrist frrist commented Jun 12, 2019

Wat

This PR repackages the way FAST is laid out to make it more friendly to work with deps
This PR adds GetFunds to the environment interface and modifies DevNet and MemoryGenesis to impl it.

@frrist frrist requested a review from travisperson June 12, 2019 21:03
@frrist frrist force-pushed the feat/repackage-fast branch from 8df4d55 to c8d373f Compare June 12, 2019 21:05
@codecov-io
Copy link

codecov-io commented Jun 12, 2019

Codecov Report

Merging #2919 into master will decrease coverage by 1%.
The diff coverage is 6%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2919     +/-   ##
========================================
- Coverage      43%     41%     -2%     
========================================
  Files         210     136     -74     
  Lines       12466    8278   -4188     
========================================
- Hits         5389    3436   -1953     
+ Misses       6262    4259   -2003     
+ Partials      815     583    -232
Impacted Files Coverage Δ
tools/fast/fastesting/basic.go 0% <0%> (ø) ⬆️
tools/fast/environment/environment_devnet.go 0% <0%> (ø)
...ols/fast/environment/environment_memory_genesis.go 0% <0%> (ø)
tools/fast/process.go 38% <33%> (-1%) ⬇️
tools/fast/process_stderr_recorder.go 70% <80%> (-1%) ⬇️
tools/migration/internal/test_helpers.go 0% <0%> (-100%) ⬇️
core/message_pool.go 0% <0%> (-95%) ⬇️
core/message_queue.go 0% <0%> (-92%) ⬇️
protocol/hello/hello.go 2% <0%> (-87%) ⬇️
tools/migration/internal/runner.go 0% <0%> (-86%) ⬇️
... and 88 more

@frrist frrist force-pushed the feat/repackage-fast branch 2 times, most recently from 153760f to a998dfb Compare June 12, 2019 21:33
@frrist frrist requested a review from anorth June 12, 2019 23:25
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Implementation looks ok but the intent is totally opaque, so please expand on documentation before landing.

TeardownProcess(context.Context, *Filecoin) error
TeardownProcess(context.Context, *fast.Filecoin) error

// GetFunds retrieves a fixed amount of tokens from the environment.
Copy link
Member

Choose a reason for hiding this comment

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

This needs more explanation. The method only returns an error, so what does "get" mean? From where are they got? Where is the fixed amount specified? Are they guaranteed to have landed when this method returns?

Edit: having now read implementation, it would probably be better to be more explicit and call this RequestFaucetFunds or similar. The questions above still stand.

@frrist frrist self-assigned this Jun 12, 2019
@frrist frrist force-pushed the feat/repackage-fast branch from 6b52262 to 531dc1d Compare June 13, 2019 15:49
@frrist frrist force-pushed the feat/repackage-fast branch from 531dc1d to 3c7eedb Compare June 13, 2019 17:37
@frrist frrist merged commit 5db2faa into master Jun 13, 2019
@frrist frrist deleted the feat/repackage-fast branch June 13, 2019 17:54
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.

4 participants