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

Fix InitWithReset - rely on previously updated flags #7598

Merged
merged 8 commits into from Oct 22, 2020

Conversation

farazdagi
Copy link
Contributor

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

  • Normally in TestMain function is used to setup required features for package tests, e.g.:
func TestMain(m *testing.M) {
	resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{
		EnablePeerScorer: true,
	})
	
	code := m.Run()
	// os.Exit will prevent defer from being called, so calling it directly.
	resetCfg()
	os.Exit(code)
}
  • Then in individual test cases you may want to update that configuration (for example, I wanted to turn EnablePeerScorer off for just one case):
func TestBlocksFetcher_filterPeers(t *testing.T) {
	resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{
		EnablePeerScorer: false,
	})
	defer resetCfg()
        // ... skip
}
  • Now, the problem is that our current implementation of InitWithReset() will reset to &Flags{} (new empty flags object, with EnablePeerScorer assuming default value of false), instead of the one that is configured by the TestMain() (where EnablePeerScorer is set to true for the test suite).

Which issues(s) does this PR fix?

Other notes for review

@farazdagi farazdagi self-assigned this Oct 21, 2020
@farazdagi farazdagi marked this pull request as ready for review October 21, 2020 17:52
@farazdagi farazdagi requested a review from a team as a code owner October 21, 2020 17:52
@farazdagi farazdagi added Ready For Review A pull request ready for code review OK to merge labels Oct 21, 2020
@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #7598 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7598   +/-   ##
=======================================
  Coverage   61.69%   61.69%           
=======================================
  Files         422      422           
  Lines       29712    29712           
=======================================
  Hits        18332    18332           
  Misses       8440     8440           
  Partials     2940     2940           

@prylabs-bulldozer prylabs-bulldozer bot merged commit 483f7f8 into master Oct 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-init-with-reset branch October 22, 2020 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants