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

Unit test reactivation #780

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Unit test reactivation #780

wants to merge 15 commits into from

Conversation

mrpilot2
Copy link
Contributor

@mrpilot2 mrpilot2 commented Aug 1, 2015

Hi,

I have reactivated unit tests for the project.

My intention was to create a common library that is used by the application and the unit tests,
but keep the build process the same as before.

Therefore I performed the following steps:

  1. Copy qgroundcontrol.pro to apmplanner_core.pro
    • Create a static library in apmplanner_core.pro named apmplanner-core
  2. Use qgroundcontrol.pro to either build unit tests or main application
    • Create a target test in qgroundcontrol.pro that includes qgcunittest.pro
    • Removed unneeded stuff in qgcunittest.pro

These steps keep the build process the same as before, that means if no special config is added to qmake, the main application gets built.
If CONFIG+=test is added to qmake the unit tests will be built instead of the main application. The resulting executable is named apmplanner2_test.

To test the new unit test setup I have extracted the function that compares version strings from ApmFirmwareConfig and AutoUpdateCheck and implemented a new class VersionComparator and added tests for this class.

mrpilot2 added 14 commits July 26, 2015 19:12
create a static library of all apmplanner classes except of main, moved qgroundcontrol.pro to apmplanner_core.pro to keep the build process the same as before
removed old project configuration and source files from qgcunittest.pro because that is now part of the new common apmplanner-core library

added a test for the qt test framwork to make sure the new setup works as expected
versions, copied from ApmFirmwareConfig.cc

added unit tests for version comparator
in ApmFirmwareConfig.cc

This bug was already fixed in AutoUpdateCheck.cc
commit 8de0f15
… defined in the regex,

therefore it is always 2 and the if statement is useless
…rsion string in one place

fixed uninitialized variable warning
…s per group, allow muliple digits in build number
@billbonney
Copy link
Member

Thanks, Nice refactor of the compare version functions (and it should only have been in one place)

My only question is that the old regexp allowed for 1.2, 1.3, 1.4 etc optional build. with optional rc-X tag.

I'm not sure what happens with lines at https://github.com/mrpilot2/apm_planner/commit/81b1762d4c55a69badb9d1a5f12163eeb6257f72#diff-e67de0f4b1bb740829b564491a77041fR38
if you call, for example. .cap(3).toInt on an optional entity, what's the outcome?

I'll merge the code changes in, hopefully tomorrow. And now you have readied the test framework, we can write the extra test cases ;)

just quickly that would be

1.0 -> 1.1 = true
1.1 -> 1.0 = false
1.1 -> 2.1 = true
2.1 -> 1.2 = false

Thx 👍

@mrpilot2
Copy link
Contributor Author

mrpilot2 commented Aug 2, 2015

Hi,

The capture groups for build and rc-X tag are also optional, as it was in the old regex.

The function capture() always returns the number of capture groups defined in the regex. in our case captureCount() is always 6 no matter if optional groups are found or not. Optional capture groups may result in an empty string.

The regex now has ensured that the capture string contains a number or is an empty string. If it is an empty string the conversion toInt() will "fail", which results to a 0.

Summary: if you call, for example. .cap(3).toInt on an optional entity, the outcome is 0 (integer)

I have added the tests you have suggested to prove this behaviour.

@billbonney
Copy link
Member

Thanks for the PR. I have started the merge but have been slowed by a build issue on OS X . Should get the fix in at the weekend. 👍

@billbonney billbonney self-assigned this Aug 6, 2015
@billbonney
Copy link
Member

Sorry, I haven't forgotten about this PR, just swamped with other work

@billbonney
Copy link
Member

@mrpilot2 if you like to rebase these changes on master, I can merge the PR, or I can do it (but it might take little longer) I cleaned up most of my other PRs, I'm going to see if I merge all of them in the coming weeks

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.

2 participants