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

test: Introduce basic testing for the version module #1111

Merged
merged 2 commits into from
Feb 16, 2023
Merged

test: Introduce basic testing for the version module #1111

merged 2 commits into from
Feb 16, 2023

Conversation

orpheuslummis
Copy link
Contributor

@orpheuslummis orpheuslummis commented Feb 16, 2023 •
edited
Loading

Relevant issue(s)

Resolves #1110

Description

Just a few basic tests for the module.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

  • MacOS

@orpheuslummis orpheuslummis self-assigned this Feb 16, 2023
@orpheuslummis
Copy link
Contributor Author

The CI fails because the PR title ends with `. IMO it should be an acceptable character. For now I'm changing it to something else

@orpheuslummis orpheuslummis changed the title test: Introduce basic testing for version test: Introduce basic testing for the version module Feb 16, 2023
@orpheuslummis orpheuslummis added area/testing Related to any test or testing suite action/no-benchmark Skips the action that runs the benchmark. labels Feb 16, 2023
@orpheuslummis orpheuslummis added this to the DefraDB v0.5 milestone Feb 16, 2023
@orpheuslummis orpheuslummis force-pushed the orpheus/chore/basic-version-tests branch from 802772f to 5419bec Compare February 16, 2023 17:48
@orpheuslummis orpheuslummis requested a review from a team February 16, 2023 17:49
@sourcenetwork sourcenetwork deleted a comment from source-devs Feb 16, 2023
@codecov
Copy link

codecov bot commented Feb 16, 2023 •
edited
Loading

Codecov Report

Merging #1111 (8ce867a) into develop (a6995d6) will increase coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1111      +/-   ##
===========================================
+ Coverage    67.50%   67.80%   +0.29%     
===========================================
  Files          181      181              
  Lines        16551    16551              
===========================================
+ Hits         11173    11222      +49     
+ Misses        4438     4390      -48     
+ Partials       940      939       -1
Impacted Files Coverage Δ
datastore/memory/memory.go 88.73% <0.00%> (+0.93%) ⬆️
version/version.go 100.00% <0.00%> (+100.00%) ⬆️

AndrewSisley
AndrewSisley approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

LGTM. One question though before merge if that's alright :)

version/version_test.go Outdated
assert.NotEmpty(t, dv.NetProtocol)
assert.NotEmpty(t, dv.DocKeyVersions)

// because these are set by the build system, we expect them to be empty in this unit test...
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Will this test not be run on release once these variables have been set? And will this not cause the build/tests to fail for anyone running them on a release branch/tag/whatever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variables are set in the Makefile via BUILD_FLAGS and they are only passed for building defradb and not testing defradb.

For now, this allows to run the tests outside of using make.

I suggest (to myself) adding a comment indicating that assumption.

We might want to run these tests at some point in the future using BUILD_FLAGS, I'm unsure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it - thanks Orpheus :) New comment looks good too

@orpheuslummis orpheuslummis merged commit f80d8e1 into develop Feb 16, 2023
@orpheuslummis orpheuslummis deleted the orpheus/chore/basic-version-tests branch February 16, 2023 18:36
shahzadlone pushed a commit that referenced this pull request Apr 13, 2023
@orpheuslummis @shahzadlone
test: Introduce basic testing for the version module (#1111)
b074e95
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@orpheuslummis
test: Introduce basic testing for the version module (sourcenetwork…
3780436 
…#1111)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewSisley AndrewSisley AndrewSisley approved these changes

Assignees

@orpheuslummis orpheuslummis

Labels
action/no-benchmark Skips the action that runs the benchmark. area/testing Related to any test or testing suite
Projects
None yet
Milestone
DefraDB v0.5
Development

Successfully merging this pull request may close these issues.

Basic testing for version
2 participants
@orpheuslummis @AndrewSisley