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

Improve internal package tests #1227

Merged
merged 1 commit into from
Jul 30, 2021
Merged

Improve internal package tests #1227

merged 1 commit into from
Jul 30, 2021

Conversation

kevindiu
Copy link
Contributor

@kevindiu kevindiu commented May 13, 2021

Description:

This PR include the following changes:

  • using NOP logger for test
  • wrap goleak library to internal package, default ignore conflict with parallel test
  • update goleak import path
  • Update exist tests to add prefix to the environment name
  • fix internal/db/storage/blob/s3/reader/reader_test.go error
  • fix internal/file/file_test.go goleak error
  • close watcher after test in internal/file/watch/watch_test.go

The PR seems huge but most of them are updating NOP logger for test, and update the goleak import path.

Since goleak doesn't support parallel test, this PR does not update to support parallel test.
ref: uber-go/goleak#16

Related Issue:

How Has This Been Tested?:

Environment:

  • Go Version: 1.16
  • Docker Version: 19.03.8
  • Kubernetes Version: 1.18.2
  • NGT Version: 1.12.3

Types of changes:

  • Bug fix [type/bug]
  • New feature [type/feature]
  • Add tests [type/test]
  • Security related changes [type/security]
  • Add documents [type/documentation]
  • Refactoring [type/refactoring]
  • Update dependencies [type/dependency]
  • Update benchmarks and performances [type/bench]
  • Update CI [type/ci]

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Checklist:

  • I have read the CONTRIBUTING document.
  • I have checked open Pull Requests for the similar feature or fixes?
  • I have added tests and benchmarks to cover my changes.
  • I have ensured all new and existing tests passed.
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly.

@vdaas-ci
Copy link
Collaborator

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - replace the PR body by changelog details
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • /rebase - rebase master
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #1227 (b700761) into master (b898fd9) will increase coverage by 0.52%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1227      +/-   ##
==========================================
+ Coverage   17.36%   17.89%   +0.52%     
==========================================
  Files         509      512       +3     
  Lines       32259    32614     +355     
==========================================
+ Hits         5601     5835     +234     
- Misses      26352    26457     +105     
- Partials      306      322      +16     
Impacted Files Coverage Δ
internal/db/storage/blob/s3/reader/reader.go 63.33% <0.00%> (-31.12%) ⬇️
internal/net/net.go 49.18% <0.00%> (ø)
internal/net/dialer.go 79.42% <0.00%> (ø)
internal/net/option.go 89.65% <0.00%> (ø)
internal/worker/worker.go 83.33% <0.00%> (+2.08%) ⬆️
internal/file/watch/watch.go 92.92% <0.00%> (+3.03%) ⬆️
internal/config/discoverer.go 83.33% <0.00%> (+33.33%) ⬆️

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 b898fd9...b700761. Read the comment docs.

internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Show resolved Hide resolved
internal/file/watch/watch_test.go Show resolved Hide resolved
internal/db/storage/blob/s3/reader/reader_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Outdated Show resolved Hide resolved
internal/file/watch/watch_test.go Show resolved Hide resolved
internal/file/watch/watch_test.go Show resolved Hide resolved
internal/file/watch/watch_test.go Show resolved Hide resolved
@kevindiu kevindiu force-pushed the test/internal/fix-test-error branch from 79849f8 to 095d30b Compare May 21, 2021 08:24
@kevindiu kevindiu force-pushed the test/internal/fix-test-error branch from 29f730e to ea7aadf Compare May 21, 2021 11:03
@kevindiu
Copy link
Contributor Author

/rebase
/format

@vdaas-ci
Copy link
Collaborator

[REBASE] Rebase triggered by kevindiu for branch: test/internal/fix-test-error

@vdaas-ci
Copy link
Collaborator

[FORMAT] Updating license headers and formatting go codes triggered by kevindiu.

@kevindiu kevindiu force-pushed the test/internal/fix-test-error branch from 0f98fd2 to f9370fd Compare June 4, 2021 01:49
@kevindiu kevindiu changed the title [WIP] Fix internal ci error Improve internal package tests Jun 4, 2021
@kevindiu
Copy link
Contributor Author

kevindiu commented Jun 4, 2021

/format

@kevindiu kevindiu requested review from hlts2 and vankichi June 4, 2021 03:16
@kevindiu kevindiu force-pushed the test/internal/fix-test-error branch from 3245a4c to 0a9b03a Compare June 4, 2021 03:19
@vdaas-ci
Copy link
Collaborator

[REBASE] Rebase triggered by kevindiu for branch: test/internal/fix-test-error

@vdaas-ci vdaas-ci force-pushed the test/internal/fix-test-error branch from 92a252e to 8762403 Compare June 18, 2021 02:02
@kpango
Copy link
Collaborator

kpango commented Jun 23, 2021

it would be better to update header.tmpl to change import path for goleak

@kevindiu
Copy link
Contributor Author

kevindiu commented Jul 2, 2021

/rebase

@vdaas-ci
Copy link
Collaborator

vdaas-ci commented Jul 2, 2021

[REBASE] Rebase triggered by kevindiu for branch: test/internal/fix-test-error

@vdaas-ci vdaas-ci force-pushed the test/internal/fix-test-error branch from 8762403 to c7cd0ea Compare July 2, 2021 01:41
@kevindiu kevindiu force-pushed the test/internal/fix-test-error branch 2 times, most recently from 0b3402b to f08d700 Compare July 2, 2021 03:36
@kevindiu kevindiu changed the title [WIP] Improve internal package tests Improve internal package tests Jul 2, 2021
@kevindiu
Copy link
Contributor Author

kevindiu commented Jul 9, 2021

/rebase

@vdaas-ci
Copy link
Collaborator

vdaas-ci commented Jul 9, 2021

[REBASE] Rebase triggered by kevindiu for branch: test/internal/fix-test-error

@vdaas-ci vdaas-ci force-pushed the test/internal/fix-test-error branch from 86b50d4 to 0212721 Compare July 9, 2021 00:36
@kevindiu
Copy link
Contributor Author

kevindiu commented Jul 9, 2021

Since goleak does not support parallel test to use defer goleak.VerifyNone(), it only support checking it through TestMain(). S
Should we implement the following function in each package to enable parallel test

func TestMain(m *testing.M) {
	goleak.VerifyTestMain(m)
}

or should we disable parallel test and use defer goleak.VerifyNone()?

hlts2
hlts2 previously approved these changes Jul 28, 2021
hlts2
hlts2 previously approved these changes Jul 29, 2021
Signed-off-by: kpango <kpango@vdaas.org>
Copy link
Collaborator

@kpango kpango left a comment

Choose a reason for hiding this comment

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

LGTM

@kpango kpango merged commit 85f49d0 into master Jul 30, 2021
@kpango kpango deleted the test/internal/fix-test-error branch July 30, 2021 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants