testing(dot): migrate dot/sync tests to integration tests #2116

edwardmack · 2021-12-13T20:19:30Z

Changes

Migrate existing non-mocked tests to integration tests, prepend file with build tag.
Change filenames to xx_integration_test.go

Tests

go test /dot/sync/ -short -v
go test /dot/sync/ -short -v --tags=integration

Issues

closes: Migrate dot/sync tests to integration tests #2023

Primary Reviewer

@timwu20

codecov · 2021-12-16T23:29:11Z

Codecov Report

Merging #2116 (a8c0a0c) into development (368f8b6) will decrease coverage by 3.93%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           development    #2116      +/-   ##
===============================================
- Coverage        61.32%   57.39%   -3.94%     
===============================================
  Files              216      216              
  Lines            28403    28493      +90     
===============================================
- Hits             17418    16353    -1065     
- Misses            9198    10456    +1258     
+ Partials          1787     1684     -103

Flag	Coverage Δ
unit-tests	`57.39% <ø> (-3.94%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dot/sync/block_queue.go	`0.00% <0.00%> (-100.00%)`	⬇️
dot/sync/outliers.go	`0.00% <0.00%> (-96.16%)`	⬇️
dot/sync/tip_syncer.go	`0.00% <0.00%> (-83.46%)`	⬇️
dot/sync/disjoint_block_set.go	`0.00% <0.00%> (-82.56%)`	⬇️
dot/sync/test_helpers.go	`0.00% <0.00%> (-80.00%)`	⬇️
dot/sync/worker.go	`0.00% <0.00%> (-77.42%)`	⬇️
dot/sync/bootstrap_syncer.go	`0.00% <0.00%> (-74.08%)`	⬇️
dot/sync/message.go	`0.00% <0.00%> (-66.90%)`	⬇️
dot/sync/chain_sync.go	`0.00% <0.00%> (-53.73%)`	⬇️
dot/sync/chain_processor.go	`0.00% <0.00%> (-48.05%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 368f8b6...a8c0a0c. Read the comment docs.

qdm12 · 2021-12-17T12:40:31Z

@edwardmack didn't git detect the file movements ('replace')? If you didn't modify the files much, git should had detected these are just file movements, drastically reducing deltas (correct me if I'm wrong though).
If you did modify the files enough that git doesn't detect them as file moves, maybe have a single commit moving all the files (even if compilation fails) and a second one modifying them, and we can then merge (not squash and merge) your PR so git stuff is kept in the development branch (who modified what etc.)

EDIT: you might have to remove i.e. dot/sync/bootstrap_syncer_test.go for git to detect is a file movement.

dot/sync/chain_sync_integeration_test.go

dot/sync/disjoint_block_set_integeration_test.go

dot/sync/syncer_integeration_test.go

dot/sync/tip_syncer_integeration_test.go

dot/sync/chain_sync_integeration_test.go

dot/sync/syncer_integeration_test.go

dot/sync/tip_syncer_integeration_test.go

qdm12 · 2022-02-10T15:24:59Z

Also sorry to be the git maniac here, but perhaps you could interactively rebase on your first commit to have the following commits:

file movements / deletions
file changes

And then merge commits instead of squash and merge? That way file movements will be kept in history and easier to merge/rebase on for other branches (and also for git blame etc.). Otherwise git might just think it's purely new files and older files got deleted. I can help with that let me know 😉

edwardmack · 2022-03-11T22:21:55Z

@qdm12 I've interactively rebased these commits, it seems to be clearer now. Please take a look again and let me know your thoughts.

qdm12

Nice rebase! Just a few questions and suggestions to revert back to how it was to reduce deltas.

We should be able to squash and merge and git will track file movements correctly, so don't worry about adding commits 😉

dot/sync/chain_sync_integeration_test.go

dot/sync/disjoint_block_set_integeration_test.go

dot/sync/message_integeration_test.go

dot/sync/syncer_integeration_test.go

qdm12 · 2022-03-15T16:28:24Z

dot/sync/chain_sync_integeration_test.go

@@ -647,7 +650,6 @@ func TestChainSync_doSync(t *testing.T) {
 	resp := &network.BlockResponseMessage{
 		BlockData: []*types.BlockData{
 			{
-				Hash: common.Hash{0x1},


Are we sure about these removals? Does the test pass locally?

I've re-tested, and I have re-added these. I'm not sure why I removed these.

qdm12

Can you squash the last few commits in the 2nd commit, and then let's rebase and merge

Note in the future, you can stage your changes and then do git commit --amend && git push --force to bundle changes in the last commit.

qdm12

Ignore the comment above; changes are minimal so the squash and merge should track file movements properly. Let's just squash and merge it 💯

qdm12

Please add dot/sync to

gossamer/.github/workflows/integration-tests.yml

Line 28 in 19a2b4e

github.com/ChainSafe/gossamer/lib/babe,

and then let's merge this! 👍

edwardmack · 2022-04-11T14:50:50Z

Added dot/sync package to github integration workflow.

kishansagathiya

Just a few comments

Make sure to not modify irrelevant code.
Move back the line spacing that was removed.
TestChainProcessor_HandleJustification fails. If CI keeps timing out, run relevant integration tests locally and confirm if they pass.

dot/sync/disjoint_block_set_integeration_test.go

dot/sync/tip_syncer_integeration_test.go

dot/sync/chain_processor_integration_test.go

edwardmack · 2022-05-04T17:29:05Z

@kishansagathiya I've addressed PR comments, please re-review and let me know if any issues remain.

kishansagathiya

🎉

.github/workflows/integration-tests.yml

remove empty files that were moved to xx_integration_tests

fix lint issues address PR comments re-add deleted code fix TestWorkerToRequest and re-add TestChainSync_highestBlock

dot/sync/syncer_integeration_test.go

edwardmack requested review from arijitAD, EclesioMeloJunior, jimjbrettj, kishansagathiya, noot, qdm12 and timwu20 as code owners December 13, 2021 20:19

qdm12 approved these changes Jan 21, 2022

View reviewed changes

dot/sync/chain_sync_integeration_test.go Outdated Show resolved Hide resolved

dot/sync/chain_sync_integeration_test.go Outdated Show resolved Hide resolved

dot/sync/chain_sync_integeration_test.go Outdated Show resolved Hide resolved

noot reviewed Jan 21, 2022

View reviewed changes

dot/sync/disjoint_block_set_integeration_test.go Outdated Show resolved Hide resolved

edwardmack force-pushed the ed/sync_tests_to_integration branch 2 times, most recently from 2b46760 to bdee8ea Compare February 4, 2022 15:56

noot reviewed Feb 4, 2022

View reviewed changes

dot/sync/syncer_integeration_test.go Outdated Show resolved Hide resolved

noot reviewed Feb 4, 2022

View reviewed changes

dot/sync/tip_syncer_integeration_test.go Outdated Show resolved Hide resolved

noot reviewed Feb 4, 2022

View reviewed changes

dot/sync/chain_sync_integeration_test.go Outdated Show resolved Hide resolved

noot reviewed Feb 4, 2022

View reviewed changes

dot/sync/chain_sync_integeration_test.go Outdated Show resolved Hide resolved

qdm12 reviewed Feb 10, 2022

View reviewed changes

noot approved these changes Mar 3, 2022

View reviewed changes

edwardmack force-pushed the ed/sync_tests_to_integration branch 2 times, most recently from 324c1f6 to b6073f0 Compare March 11, 2022 21:53

edwardmack changed the title ~~testing (dot): migrate dot/sync tests to integration tests~~ testing(dot): migrate dot/sync tests to integration tests Mar 11, 2022

qdm12 reviewed Mar 14, 2022

View reviewed changes

qdm12 approved these changes Mar 15, 2022

View reviewed changes

qdm12 suggested changes Mar 17, 2022

View reviewed changes

qdm12 approved these changes Mar 17, 2022

View reviewed changes

qdm12 suggested changes Mar 30, 2022

View reviewed changes

qdm12 approved these changes Apr 11, 2022

View reviewed changes

kishansagathiya suggested changes Apr 19, 2022

View reviewed changes

dot/sync/disjoint_block_set_integeration_test.go Outdated Show resolved Hide resolved

dot/sync/tip_syncer_integeration_test.go Outdated Show resolved Hide resolved

dot/sync/chain_processor_integration_test.go Outdated Show resolved Hide resolved

kishansagathiya approved these changes May 5, 2022

View reviewed changes

qdm12 suggested changes May 5, 2022

View reviewed changes

.github/workflows/integration-tests.yml Show resolved Hide resolved

edwardmack added 8 commits May 5, 2022 10:34

move tests to integeration files / delete empty files

0e41271

remove empty files that were moved to xx_integration_tests

add build integration tag, lint and address PR comments

20d1870

fix lint issues address PR comments re-add deleted code fix TestWorkerToRequest and re-add TestChainSync_highestBlock

address PR comments

0a12d5e

address PR comments, revert code

62a1a96

fix TestDisjointBlockSet test

1a9dbe1

Add dot/sync package to github integration workflow

1f06bbc

address PR comments

23e0204

fix test, add reference to mock telemetry client

6880a9b

edwardmack force-pushed the ed/sync_tests_to_integration branch from f9bf802 to 6880a9b Compare May 5, 2022 14:36

edwardmack requested review from qdm12 and removed request for arijitAD May 5, 2022 14:58

qdm12 suggested changes May 12, 2022

View reviewed changes

dot/sync/syncer_integeration_test.go Outdated Show resolved Hide resolved

edwardmack added 2 commits May 12, 2022 14:54

re-add deleted go:generate tag

7a45a3d

fix TestChainSync_doSync test

a8c0a0c

edwardmack requested a review from qdm12 May 12, 2022 20:30

timwu20 approved these changes May 17, 2022

View reviewed changes

qdm12 approved these changes May 17, 2022

View reviewed changes

edwardmack merged commit e29f90c into development May 17, 2022

edwardmack deleted the ed/sync_tests_to_integration branch May 17, 2022 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testing(dot): migrate dot/sync tests to integration tests #2116

testing(dot): migrate dot/sync tests to integration tests #2116

edwardmack commented Dec 13, 2021

codecov bot commented Dec 16, 2021 •

edited

Loading

qdm12 commented Dec 17, 2021 •

edited

Loading

qdm12 commented Feb 10, 2022

edwardmack commented Mar 11, 2022

qdm12 left a comment

qdm12 Mar 15, 2022

edwardmack May 12, 2022

qdm12 left a comment •

edited

Loading

qdm12 left a comment

qdm12 left a comment

edwardmack commented Apr 11, 2022

kishansagathiya left a comment •

edited

Loading

edwardmack commented May 4, 2022

kishansagathiya left a comment

testing(dot): migrate dot/sync tests to integration tests #2116

testing(dot): migrate dot/sync tests to integration tests #2116

Conversation

edwardmack commented Dec 13, 2021

Changes

Tests

Issues

Primary Reviewer

codecov bot commented Dec 16, 2021 • edited Loading

Codecov Report

qdm12 commented Dec 17, 2021 • edited Loading

qdm12 commented Feb 10, 2022

edwardmack commented Mar 11, 2022

qdm12 left a comment

Choose a reason for hiding this comment

qdm12 Mar 15, 2022

Choose a reason for hiding this comment

edwardmack May 12, 2022

Choose a reason for hiding this comment

qdm12 left a comment • edited Loading

Choose a reason for hiding this comment

qdm12 left a comment

Choose a reason for hiding this comment

qdm12 left a comment

Choose a reason for hiding this comment

edwardmack commented Apr 11, 2022

kishansagathiya left a comment • edited Loading

Choose a reason for hiding this comment

edwardmack commented May 4, 2022

kishansagathiya left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 16, 2021 •

edited

Loading

qdm12 commented Dec 17, 2021 •

edited

Loading

qdm12 left a comment •

edited

Loading

kishansagathiya left a comment •

edited

Loading