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

Disable a test that broke the current build #655

Merged
merged 1 commit into from
May 5, 2021
Merged

Disable a test that broke the current build #655

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions production/tools/gaia_translate/tests/test.ruleset
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#include "gaia/logger.hpp"
#include "gaia/events.hpp"

// #define TEST_FAILURES
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused about this approach of disabling the tests. Why have this commented line here? It's not as if you'd ever want to uncomment it, right?

And then the #ifdef TEST_FAILURES doesn't sound intuitive - the code is enabled if there are test failures?

I think it would be more descriptive to call the macro variable something like ENABLE_FAILING_TESTS and then enclose the ruleset in:

#ifndef ENABLE_FAILING_TESTS
...
#endif

I wouldn't keep any commented out define line either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With Gregory enhancing gaiat while I am writing gaiat tests, I try to get ahead of him with tests that won't pass yet. For bettor or worse, I have a convention in the test files to disable tests that don't pass yet. The disabling of this one test simply follows the pattern that is already in place. It's a reminder to me of tests to re-test when there is a new commit from Gregory. Eventually, all of the #ifdef statements will be gone. BTW the name TEST_FAILURES means "test the failures" and is facilitated by uncommenting the #define. This is transitional as Gregory and I move forward with gaiat.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ambiguity is that in this context "test" is used more like a noun than a verb.

Also, do you really want to test all the failures in the file, or do you want to test them one by one (in which case the top define is irrelevant)?

Copy link
Contributor

@chuan chuan May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is it possible to make this test part of Gregory's branch? You can update the PR to merge the change to his branch. Logistic wise, we won't need to re-enable this after Gregory's change is in and the test can be used to test gaiat change in the local branch before merging to master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to work with Gregory to find better ways to synchronize the language enhancements with the tests. Obviously this shouldn't interfere with the build. Thanks for the suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.


int g_rule_called = 0;
int g_insert_called = 0;
int g_update_called = 0;
Expand Down Expand Up @@ -73,6 +75,7 @@ ruleset test3
}
}

#ifdef TEST_FAILURES
ruleset testE13
{
OnInsert(sensor)
Expand All @@ -81,6 +84,7 @@ ruleset testE13
daily += /sensor.value;
}
}
#endif

ruleset test21
{
Expand Down