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

Use a struct over global vars in default_syncer_test #2700

Merged
merged 5 commits into from
May 23, 2019
Merged

Conversation

mslipper
Copy link
Contributor

@mslipper mslipper commented May 1, 2019

Closes #2116. All *Test methods initialize a struct that contains the syncer params, which is in turn passed to all the other initialization/require* methods. Let me know if there's an alternative approach you'd like me to implement.

@mslipper
Copy link
Contributor Author

mslipper commented May 1, 2019

Hmm, quite a few conflicts - if this is generally the right track let me know and I'll fix them. Would prefer to know the solution is OK before resolving.

@mslipper mslipper changed the title Display error when address is not a miner address Use a struct over global vars in default_syncer_test May 2, 2019
@frrist frrist requested a review from ZenGround0 May 2, 2019 16:12
@ZenGround0
Copy link
Contributor

ZenGround0 commented May 2, 2019

@mslipper this is definitely the right approach and generally great. As you've probably already seen the conflicts are most likely all related to our new usage pattern around the require and assert packages introduced in PR #2657. These should be mostly orthogonal and hopefully make the rebase process simple (though surely tedious).

@mslipper
Copy link
Contributor Author

mslipper commented May 2, 2019

Excellent. I'll resolve the conflicts shortly.

@mslipper mslipper force-pushed the issue-2116 branch 2 times, most recently from 57ab497 to 2ce5cbb Compare May 2, 2019 20:37
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

This is great, thank you for cleaning up this mess

chain/default_store_test.go Outdated Show resolved Hide resolved
Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

Once the flags are added back, LGTM

chain/default_store_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

once @frrist 's comments are addressed, gtg... thanks for taking on this tedious task. so much better now

chain/default_syncer_test.go Show resolved Hide resolved
@ZenGround0 ZenGround0 merged commit e1985e8 into master May 23, 2019
deaswang pushed a commit to filcloud/go-filecoin that referenced this pull request May 24, 2019
…t#2700)

* Use a struct over global vars in default_syncer_test

Closes filecoin-project#2116.

* Remove bad unit test flags

* Add UnitTest flags
@ZenGround0 ZenGround0 deleted the issue-2116 branch October 29, 2019 14:50
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.

Remove global variables from default_syncer_test and default_store_test
4 participants