add test for the core of Falcon - #105
Conversation
|
Comment by @MarineChap on April 08, 2021 at 02:44 PM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
|
Comment by @MarineChap on April 09, 2021 at 01:12 PM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
|
Comment by @MarineChap on April 09, 2021 at 01:27 PM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
|
Comment by @MarineChap on April 09, 2021 at 01:43 PM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
|
Comment by @MarineChap on April 09, 2021 at 02:08 PM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
|
Comment by @MarineChap on April 09, 2021 at 02:22 PM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
|
Comment by @MarineChap on April 09, 2021 at 04:57 PM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
Comment by @fkloosterman on April 10, 2021 at 10:27 AM UTC:
It looks like you added arguments that are specific for testing. Can we differentiate between test builds and release builds that use more aggressive optimization? |
|
|
||
| MESSAGE("Build TEST") | ||
| include(CTest) | ||
| enable_testing() |
There was a problem hiding this comment.
Comment by @fkloosterman on April 10, 2021 at 10:35 AM UTC:
💬 Code comment on
CMakeLists.txt(line 172) (commit:2beb3ca)
Does this mean that tests are always enabled, or is there a way to skip the tests?
There was a problem hiding this comment.
Comment by @MarineChap on April 12, 2021 at 11:29 AM UTC:
💬 Code comment on
CMakeLists.txt(line 172) (commit:2beb3ca)
yes the core tests are always enabled.
The testing option will be used later for enable tests in the extensions which cannot be build at the same time as falcon+core-test.
The debug release would come useful for this point too. I guess it is logical to always have the core-test built in the debug release, right ?
There was a problem hiding this comment.
Comment by @fkloosterman on April 12, 2021 at 04:44 PM UTC:
💬 Code comment on
CMakeLists.txt(line 172) (commit:2beb3ca)
@MarineChap does this mean that every time you try to compile falcon, the tests are automatically built? Once there are many tests, I can imagine this may take some time.
There was a problem hiding this comment.
Comment by @MarineChap on April 12, 2021 at 04:47 PM UTC:
💬 Code comment on
CMakeLists.txt(line 172) (commit:2beb3ca)
@fkloosterman yes but if we have a debug and a release release, it will compile them only in debug release. And in debug release, by definition you need to see if the tests are working.
But I can definitely change it to put the option testing for the whole test and then a second option for choosing which type of tests to build (core or extension).
There was a problem hiding this comment.
Comment by @fkloosterman on April 12, 2021 at 05:50 PM UTC:
💬 Code comment on
CMakeLists.txt(line 172) (commit:2beb3ca)
@MarineChap I may misunderstand, but how does it work when you are implementing a new feature and you compile often to quickly test your work? I would imagine you want to run the tests only after you have a reached a decent implementation of your new feature?
There was a problem hiding this comment.
Comment by @MarineChap on April 12, 2021 at 06:05 PM UTC:
💬 Code comment on
CMakeLists.txt(line 172) (commit:2beb3ca)
@fkloosterman In fact no, the best test approach is a test-driven development (TDD) approach but that’s honestly very difficult. Test failing first, then new feature then verify the test works.
At the very least, I would use it by doing incremental refactoring with running tests at each step to know immediately when you are breaking something.
By only verifying test or implementing new tests at the very end of a new feature, it is more a burden than something helpful.
Also failing tests do not mean you cannot compile it.
Worse case, you can still compile only the falcon target (and its dependencies) :
cmake --build . --target falcon
|
|
||
| std::vector<std::string> result = expandProcessorName("processor"); | ||
| EXPECT_EQ(result[0], "processor"); | ||
| EXPECT_EQ(result.size(), 1); |
There was a problem hiding this comment.
Comment by @fkloosterman on April 10, 2021 at 10:52 AM UTC:
💬 Code comment on
tests/graph_parser_test.cpp(line 31) (commit:2beb3ca)
You should probably test size before trying to access items in the result. The same is true for other tests below.
|
|
||
| namespace { | ||
|
|
||
| TEST(expandProcessorName, DefaultName) { |
There was a problem hiding this comment.
Comment by @fkloosterman on April 10, 2021 at 10:53 AM UTC:
💬 Code comment on
tests/graph_parser_test.cpp(line 27) (commit:2beb3ca)
Should we also add tests that are expected to fail or raise an exception?
There was a problem hiding this comment.
Comment by @MarineChap on April 12, 2021 at 11:50 AM UTC:
💬 Code comment on
tests/graph_parser_test.cpp(line 27) (commit:2beb3ca)
I think it depends. The runtime error due to bad parsing if it is not pointing to some meaningful possible wrong messages are cluttering the meaning of the tests.
I will add a test to fail when the same identifiers are given multiple times but for the others, I don’t see useful wrong examples worth highlighting in the tests.
Do you see particular use cases where we could wrongfully give a bad format?
(not sure I am clear here - sorry)
There was a problem hiding this comment.
Comment by @fkloosterman on April 12, 2021 at 05:33 PM UTC:
💬 Code comment on
tests/graph_parser_test.cpp(line 27) (commit:2beb3ca)
@MarineChap I was thinking about the proper handling of invalid characters and ill-formatted number ranges (e.g. “name [1..2]” or “name (10-4)”)
There was a problem hiding this comment.
Comment by @MarineChap on April 12, 2021 at 06:08 PM UTC:
💬 Code comment on
tests/graph_parser_test.cpp(line 27) (commit:2beb3ca)
@fkloosterman Okay I can add a test for this two formats.
| } | ||
|
|
||
| TEST(ParseConnectionRules, ExpandPartIdentifiers) { | ||
| ConnectionRule rules = parseConnectionRule("source.hp = f:ripple_filter.p:data.s:1"); |
There was a problem hiding this comment.
Comment by @fkloosterman on April 10, 2021 at 11:04 AM UTC:
💬 Code comment on
tests/graph_parser_test.cpp(line 116) (commit:2beb3ca)
I would change the name of the test to something like “ExplicitPartIdentifiers”
|
|
||
| EXPECT_THROW(parseConnectionRule("source.hp = f:ripple_filter.s:3.s:1"), std::runtime_error); | ||
| } | ||
|
|
There was a problem hiding this comment.
Comment by @fkloosterman on April 10, 2021 at 11:04 AM UTC:
💬 Code comment on
tests/graph_parser_test.cpp(line 137) (commit:2beb3ca)
This test could be named “ExpandPorts”
| cmdline::parser parser; | ||
| parser.add<string>("logpath", 'l', "logging path of the test", false, "$HOME/log"); | ||
| parser.add("noscreenlog", '\0', "disable logging to screen"); | ||
| parser.parse_check(argc, argv); |
There was a problem hiding this comment.
Comment by @fkloosterman on April 10, 2021 at 11:23 AM UTC:
💬 Code comment on
tests/t_main.cpp(line 36) (commit:2beb3ca)
How can these command line options be changed by a developer when running the tests?
There was a problem hiding this comment.
Comment by @MarineChap on April 12, 2021 at 11:20 AM UTC:
💬 Code comment on
tests/t_main.cpp(line 36) (commit:2beb3ca)
It is useful when running the executable test alone.
However, I looked into it to how to pass the option command line with ctest command. It is a pending feature request with some ugly/complicated workarounds.
What do you think? Do you prefer that I looked more closely how to add this options in ctest ?
There was a problem hiding this comment.
Comment by @fkloosterman on April 12, 2021 at 05:46 PM UTC:
💬 Code comment on
tests/t_main.cpp(line 36) (commit:2beb3ca)
@MarineChap it is not clear if anyone is actively working on the feature. For now, I suppose we just have to rely on sensible default values when running the tests automatically.
|
Comment by @MarineChap on April 12, 2021 at 12:29 PM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
| g3::log_levels::set(DEBUG, true); | ||
| if (!parser.exist("noscreenlog")) { | ||
| worker->addSink(std::make_unique<ScreenSink>(), | ||
| &ScreenSink::ReceiveLogMessage); |
There was a problem hiding this comment.
Comment by @fkloosterman on April 10, 2021 at 11:29 AM UTC:
💬 Code comment on
tests/t_main.cpp(line 51) (commit:2beb3ca)
argc and argv still contain the options specific for logging that were parsed above. Is there a possibility that there is interference?
Since InitGoogleTest removes the options that it recognizes, can it be called first before parsing the logging options?
|
Comment by @MarineChap on April 15, 2021 at 11:30 AM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
|
Comment by @MarineChap on April 27, 2021 at 08:02 PM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
|
Comment by @MarineChap on June 22, 2021 at 09:29 AM UTC: (Travis) Great job! Your build looks valid, you can merge your code! |
|
marine chaput opened the pull request on April 08, 2021 at 11:40 AM UTC |
|
New commit added to PR: 064d0c1a0544 by marine chaput on April 08, 2021 at 11:40 AM UTC |
|
New commit added to PR: 533a3849583d by marine chaput on April 08, 2021 at 11:45 AM UTC |
|
New commit added to PR: 87e7301b97d0 by marine chaput on April 08, 2021 at 11:47 AM UTC |
|
New commit added to PR: 20a228cf7036 by marine chaput on April 08, 2021 at 12:03 PM UTC |
|
New commit added to PR: 818673d06eff by marine chaput on April 08, 2021 at 01:43 PM UTC |
|
New commit added to PR: 0121ee5136a5 by marine chaput on April 08, 2021 at 01:49 PM UTC |
|
New commit added to PR: 0ff1d8e35e2d by marine chaput on April 08, 2021 at 01:56 PM UTC |
|
New commit added to PR: 3e9ae02afe32 by marine chaput on April 08, 2021 at 02:01 PM UTC |
|
New commit added to PR: 81263d75488e by marine chaput on April 08, 2021 at 02:07 PM UTC |
|
New commit added to PR: e33020983bbe by marine chaput on April 08, 2021 at 02:18 PM UTC |
|
New commit added to PR: d1537188ed2a by marine chaput on April 08, 2021 at 02:23 PM UTC |
|
New commit added to PR: 5fe7339e27cc by marine chaput on April 08, 2021 at 02:28 PM UTC |
|
PR updated by marine chaput on April 08, 2021 at 02:40 PM UTC:
|
|
New commit added to PR: aa846763d2cc by marine chaput on April 09, 2021 at 09:24 AM UTC |
|
New commit added to PR: 1b49345b511a by marine chaput on April 09, 2021 at 12:30 PM UTC |
|
New commit added to PR: 4df947d27d0b by marine chaput on April 09, 2021 at 01:07 PM UTC |
|
New commit added to PR: 5b427163be83 by marine chaput on April 09, 2021 at 01:22 PM UTC |
|
PR updated by marine chaput on April 09, 2021 at 01:38 PM UTC:
|
|
New commit added to PR: 50b00870eada by marine chaput on April 09, 2021 at 02:03 PM UTC |
|
New commit added to PR: dee1531f3000 by marine chaput on April 09, 2021 at 02:17 PM UTC |
|
New commit added to PR: 005cbd997909 by marine chaput on April 09, 2021 at 04:52 PM UTC |
|
New commit added to PR: b04671ae2d48 by marine chaput on April 12, 2021 at 11:53 AM UTC |
|
New commit added to PR: 0ccb9bf30903 by marine chaput on April 27, 2021 at 07:57 PM UTC |
|
New commit added to PR: 2da725285fc5 by marine chaput on June 22, 2021 at 09:25 AM UTC |
|
New commit added to PR: 85c61bf by marine chaput on June 25, 2021 at 10:38 AM UTC |
|
New commit added to PR: 85c61bf by marine chaput on July 23, 2021 at 07:55 AM UTC |
1 similar comment
|
New commit added to PR: 85c61bf by marine chaput on July 23, 2021 at 07:55 AM UTC |
|
New commit added to PR: a7b0021 by marine chaput on August 03, 2021 at 02:04 PM UTC |
|
New commit added to PR: 2beb3ca by marine chaput on August 03, 2021 at 02:14 PM UTC |
1 participant
Migrated from Bitbucket
[x] Add test structure
[/ ] Add tests for the core only (no extension tests here)
[x ] Add test pipeline
Coverage report : https://coveralls.io/bitbucket/kloostermannerflab/falcon-core