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

[test] fixes unit-tests for testing version string with pre-release #1749

Merged
merged 2 commits into from
Apr 18, 2019

Conversation

technobly
Copy link
Member

@technobly technobly commented Apr 12, 2019

Problem

  • Unit Tests do not expect Alpha or Beta pre-release strings in addition to RC.
  • TEST=wiring/no_fixture test SYSTEM_02 has the same issue
  • Upgrade-downgrade.sh needs updating for 1.2.0-beta.1

Solution

  • Give the Unit-Tests some help in determining which type of string format is is testing by looking for expected strings, before the format is tested.
  • Update TEST=wiring/no_fixture test SYSTEM_02 similarly
  • Also changed test type from REQUIRE to REQUIRE_THAT to give a much better failure error!
  • Similarly updated SYSTEM_02 test type from assertTrue() to assertEqual() to give a much better failure error!
  • Updated upgrade-downgrade.sh to 1.2.0-beta.1

REQUIRE

// CODE
REQUIRE(strcmp(expected,info.versionString)==0);

// OUTPUT
REQUIRE( strcmp(expected,info.versionString)==0 )
with expansion:
  45 == 0

REQUIRE_THAT

// CODE
// using Catch::Matchers::Equals;
REQUIRE_THAT( expected, Equals(info.versionString));

// OUTPUT
../../../user/tests/unit/system_task.cpp:90: FAILED:
  REQUIRE_THAT( expected Equals(info.versionString) )
with expansion:
  "1.2.0-alpha.2" equals: "1.2.0"

Steps to Test

Run Unit Tests from the root directory (firmware $):

source ci/install_boost.sh
ci/build_boost.sh
cd user/tests/unit && make -s clean all

All tests should pass with these version strings:

1.2.0-alpha.1
1.2.0-alpha.63
1.2.0-beta.1
1.2.0-beta.63
1.2.0-rc.1
1.2.0-rc.63
1.2.0

And these defines

#define SYSTEM_VERSION_v120ALPHA1  SYSTEM_VERSION_ALPHA(1, 2, 0, 1)
#define SYSTEM_VERSION_v120ALPHA63 SYSTEM_VERSION_ALPHA(1, 2, 0, 63)
#define SYSTEM_VERSION_v120BETA1    SYSTEM_VERSION_BETA(1, 2, 0, 1)
#define SYSTEM_VERSION_v120BETA63   SYSTEM_VERSION_BETA(1, 2, 0, 63)
#define SYSTEM_VERSION_v120RC1        SYSTEM_VERSION_RC(1, 2, 0, 1)
#define SYSTEM_VERSION_v120RC63       SYSTEM_VERSION_RC(1, 2, 0, 63)
#define SYSTEM_VERSION_v120      SYSTEM_VERSION_DEFAULT(1, 2, 0)
#define SYSTEM_VERSION SYSTEM_VERSION_v120

And will fail with these strings, or mismatched string/versions

1.2.0-rc.64
#define SYSTEM_VERSION_v120RC64       SYSTEM_VERSION_RC(1, 2, 0, 64)

#define SYSTEM_VERSION_v120TEST1       0x0102000C0
#define SYSTEM_VERSION_v120TEST2      0x0102000D0
#define SYSTEM_VERSION_v120TEST3       0x0102000FE

Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • N/A Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

  • [internal] [test] fixes unit-tests for testing version string with pre-release #1749
  • [internal] [test] update upgrade-downgrade.sh to 1.2.0-beta.1 #1749

@technobly technobly added this to the 1.2.0-beta.1 milestone Apr 12, 2019
@technobly technobly changed the title Fix/unit tests pre-releases [test] fixes unit-tests for testing version string with pre-release Apr 12, 2019
@sergeuz
Copy link
Member

sergeuz commented Apr 15, 2019

The test will still fail if a given Device OS version has alpha, beta and candidate releases. How about changing our internal version numbering scheme as well? For example:

#define SYSTEM_VERSION_1_2_0_ALPHA_1 0x010200a1
#define SYSTEM_VERSION_1_2_0_BETA_1  0x010200b1
#define SYSTEM_VERSION_1_2_0_RC_1    0x010200c1
#define SYSTEM_VERSION_1_2_0         0x010200ff

@technobly
Copy link
Member Author

@sergeuz can you explain what you mean by this "The test will still fail if a given Device OS version has alpha, beta and candidate releases."

I don't see how a single release can have a version string that is -alpha.x AND -beta.x AND -rc.x. It's mutually exclusive.

The plan is to treat the lowest byte as a counter starting at 1 for alpha.1, and incrementing for as many alpha's, then beta's then rc's as necessary in that order only. We talked about changing the default release to use 0xFF to ensure it was greater than all pre-release types.

I also don't see a reason to change the formatting of the SYSTEM_VERSION defines.

Example

#define SYSTEM_VERSION_v120ALPHA1 0x01020001
#define SYSTEM_VERSION_v120ALPHA2 0x01020002
#define SYSTEM_VERSION_v120BETA1  0x01020003
#define SYSTEM_VERSION_v120BETA2  0x01020004
#define SYSTEM_VERSION_v120RC1    0x01020005
#define SYSTEM_VERSION_v120RC2    0x01020006
#define SYSTEM_VERSION_v120       0x010200ff
#define SYSTEM_VERSION            SYSTEM_VERSION_v120

@sergeuz
Copy link
Member

sergeuz commented Apr 16, 2019

can you explain what you mean by this "The test will still fail if a given Device OS version has alpha, beta and candidate releases."

@technobly Given the following defines, your test will incorrectly expect the version string to be 1.2.0-beta.3:

#define SYSTEM_VERSION_v120ALPHA1 0x01020001
#define SYSTEM_VERSION_v120ALPHA2 0x01020002
#define SYSTEM_VERSION_v120BETA1  0x01020003
#define SYSTEM_VERSION            SYSTEM_VERSION_v120BETA1
#define SYSTEM_VERSION_STRING     "1.2.0-beta.1"

@technobly
Copy link
Member Author

technobly commented Apr 16, 2019

Thanks for the clarification. Hmm... on one hand I'd like to keep the door open to easily incrementing the lowest byte so we have plenty of room to do so. 0.8.0 made it up to rc.27 which wouldn't have room with the above scheme. If we reserved the TWO MSB's for 4 variations of pre-releases that would allow for 64 different types of alpha, beta and rc. Just a bit more to think about when crafting these version defines (but the test would catch any errors in encoding and it has in the past!), and you wouldn't be able to definitively say BETA1 came after the last ALPHAx for example, but I'm not sure when you'd need to make a test like that. Being able to test in user firmware if the system version is definitively alpha, beta, rc or default seems very powerful though for obvious reasons.

How about:

#define SYSTEM_VERSION_v120ALPHA1 0x01020001
#define SYSTEM_VERSION_v120ALPHA2 0x01020002
#define SYSTEM_VERSION_v120BETA1  0x01020041
#define SYSTEM_VERSION_v120BETA2  0x01020042
#define SYSTEM_VERSION_v120RC1    0x01020081
#define SYSTEM_VERSION_v120RC2    0x01020082
#define SYSTEM_VERSION_v120       0x010200ff
// We can reserve 0xXXYYZZC0 ~ 0x......FE for a different type of pre-release,
// or let RC's use 127, where default is 0x......FF
#define SYSTEM_VERSION            SYSTEM_VERSION_v120

@sergeuz
Copy link
Member

sergeuz commented Apr 16, 2019

Using bits 6-7 of a version number to encode the version type sounds good to me as well. Let's also introduce a convenience macro to encode those version numbers, for example:

#define SYSTEM_VERSION_v120ALPHA1 SYSTEM_VERSION_ALPHA(1, 2, 0, 1)
#define SYSTEM_VERSION_v120ALPHA2 SYSTEM_VERSION_ALPHA(1, 2, 0, 2)
#define SYSTEM_VERSION_v120BETA1  SYSTEM_VERSION_BETA(1, 2, 0, 1)

@technobly technobly added ready to merge PR has been reviewed and tested and removed needs review labels Apr 18, 2019
@technobly technobly merged commit 688d4f2 into release/v1.2.0 Apr 18, 2019
@technobly technobly deleted the fix/unit-tests-prereleases branch April 18, 2019 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug internal ready to merge PR has been reviewed and tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants