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.

Sign up for GitHub

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

Jump to bottom

node tests: Use in memory badger in unit tests #347

Merged
merged 6 commits into from
May 20, 2021
Merged

node tests: Use in memory badger in unit tests #347

merged 6 commits into from
May 20, 2021

Conversation

liamsi
Copy link
Member

@liamsi liamsi commented May 20, 2021 •
edited
Loading

This PR:

  • Uses an in-memory DB for unit tests (still badger though)
  • increases some timeouts that help with test flakyness
  • Skips a test that was a PITA (and is completely unrelated to the correctness of the consensus / LL or anything we are tying to do right now and in the next few months)

@liamsi liamsi force-pushed the ismail/use-badger-in-memory branch from 8c55a69 to a5d8bc2 Compare May 20, 2021 12:06
@liamsi liamsi force-pushed the ismail/use-badger-in-memory branch from a5d8bc2 to 11d1529 Compare May 20, 2021 13:39
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (hlib/add-ipfs-cli@a55bf10). Click here to learn what that means.
The diff coverage is n/a.

@@                 Coverage Diff                  @@
##             hlib/add-ipfs-cli     #347   +/-   ##
====================================================
  Coverage                     ?   62.06%           
====================================================
  Files                        ?      263           
  Lines                        ?    22971           
  Branches                     ?        0           
====================================================
  Hits                         ?    14257           
  Misses                       ?     7207           
  Partials                     ?     1507

liamsi
liamsi commented May 20, 2021
View reviewed changes
node/node_test.go
@@ -136,18 +159,19 @@ func TestNodeSetAppVersion(t *testing.T) {
}

func TestNodeSetPrivValTCP(t *testing.T) {
t.Skip("TODO(ismail): Mock these conns using net.Pipe instead")
Copy link
Member Author

Choose a reason for hiding this comment

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

I do not fully understand why this fails more often now. But IMHO, this should not really dial tcp://" + testFreeAddr(t) but instead use net.Pipe() or sth similar. This would require some changes in the node code so it can deal with the pipe connection too. Either way, this code is highly orthogonal and we can come back to it (much) later. For now, it is just a timesink ...

Copy link
Member

Choose a reason for hiding this comment

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

True

Copy link
Member

Choose a reason for hiding this comment

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

So this is an actual fix and inmemdb does have any effect?

Copy link
Member Author

Choose a reason for hiding this comment

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

The inmemdb helps with some timeouts, too. And tests will be a tiny bit faster ...

@liamsi liamsi marked this pull request as ready for review May 20, 2021 15:31
@liamsi liamsi requested a review from evan-forbes as a code owner May 20, 2021 15:31
@liamsi liamsi requested review from tac0turtle and Wondertan and removed request for tac0turtle May 20, 2021 15:38
Wondertan
Wondertan reviewed May 20, 2021
View reviewed changes
node/node_test.go
@@ -136,18 +159,19 @@ func TestNodeSetAppVersion(t *testing.T) {
}

func TestNodeSetPrivValTCP(t *testing.T) {
t.Skip("TODO(ismail): Mock these conns using net.Pipe instead")
Copy link
Member

Choose a reason for hiding this comment

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

So this is an actual fix and inmemdb does have any effect?

p2p/transport_test.go
@@ -615,7 +615,7 @@ func TestTransportHandshake(t *testing.T) {
t.Fatal(err)
}

ni, err := handshake(c, 20*time.Millisecond, emptyNodeInfo())
ni, err := handshake(c, 100*time.Millisecond, emptyNodeInfo())
Copy link
Member

Choose a reason for hiding this comment

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

Is that a failed attempt to fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that was flaky too and it helped.

liamsi
liamsi commented May 20, 2021
View reviewed changes
node/node_test.go Outdated Show resolved Hide resolved
liamsi
liamsi commented May 20, 2021
View reviewed changes
node/node_test.go Outdated Show resolved Hide resolved
@liamsi liamsi merged commit f828736 into hlib/add-ipfs-cli May 20, 2021
@liamsi liamsi deleted the ismail/use-badger-in-memory branch May 20, 2021 15:47
cmwaters pushed a commit that referenced this pull request Mar 13, 2023
@mergify @lasarojc
QA: v0.34 - 1/3 TM 2/3 CMT (#297) (#347)
5ffd6a7 
* adding the 2/3 comet 1/3 tm case, without explanations

* moving experiment to folder to standardize names

* Description of how to run the prometheus plotter file

* regenerating figures and deleting the old ones

* moving the reporting scripts to this repo

* Updating CMT1/3 TM2/3 latency figures

* Updating prometheus metrics for cmt1 tm2 case

* Updating scripts

* extending the window to capture the long tail of experiments

* improved plotter, let's you include the average in the graph

* Updating prometheus metrics for cmt1 tm2 case

* Revert to referencing TM where it makes sense

* Updating prometheus metrics for cmt2 tm1 case

* Updating prometheus metrics for cmt2 tm1 case and references in the README.

* Updating prometheus metrics for cmt1 tm2 case

* Fixing section levels. Changed results to table

* reducing time window to 20s

* reducing time window to 20s

* Start splitting the files

* reducing time window to 20s in the script

* Update the data and comments for homogeneous network (#312)

* Changes to homogeneous results. First version

* Tweak to plotting script

* Revert "Tweak to plotting script"

* Updated homogeneous text and script

* Splitting the README.md

* Updating the instructions to plotters

* Update the latency plotter to print the experiment times in UTC and update the reference to specific experiments in the text

* Final plots for homogeneous

* Extracted and added baseline data (oct 2022) to the report

* Polishing the text

* Removed two unsued .png files

* Rename homogeneous latencies

---------

Co-authored-by: Lasaro <[email protected]>

* making file names consitent with script output and across sections. Removing files not needed. Reorganizing some sections of the report

* Plot to the imgs folder. Don't block between plots.

* Adding report for half-half configuration. Update scripts include the test.

* Fix typo in the `requirements.txt` file.
Expand the explantion of the `latency_plotter.py` script

* Moving instructions to run  to the reporting

* Small tweaks in the text

---------

Co-authored-by: Sergio Mena <[email protected]>
(cherry picked from commit 2ccf684)

# Conflicts:
#	docs/qa/README.md

Co-authored-by: Lasaro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tac0turtle tac0turtle tac0turtle left review comments

@Wondertan Wondertan Wondertan approved these changes

@evan-forbes evan-forbes Awaiting requested review from evan-forbes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liamsi @codecov-commenter @Wondertan @tac0turtle