Some cleanup for new executor test code, which accidentally fixes ASAN failures in ExecutorTestHelper #11283
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsteemann
reviewed
Mar 17, 2020
| @@ -74,7 +74,6 @@ set(ARANGODB_TESTS_SOURCES | |||
| Aql/SortLimit-test.cpp | |||
| Aql/SpliceSubqueryOptimizerRuleTest.cpp | |||
| Aql/SplicedSubqueryIntegrationTest.cpp | |||
| Aql/SubqueryExecutorTest.cpp | |||
There was a problem hiding this comment.
mmmh, why does this fix remove the SubqueryExecutorTest and its 500 lines of test code?
Is that intentional? If so, why?
There was a problem hiding this comment.
The test was not doing anything (most of the test code was #if 0d out), and rather than pretend that such a test is there, I decided to delete it.
We have an internal planning ticket to implement tests for this executor.
…t-asan-fixes-cleanup
|
@markuspf : just tried this branch and ran the tests with it. I think this is a side effect of something else being broken. |
…/arangodb into bug-fix/test-asan-fixes-cleanup
|
I tried the latest version and it still fails, though much later now: |
…/arangodb into bug-fix/test-asan-fixes-cleanup
markuspf
changed the title
…/arangodb into bug-fix/test-asan-fixes-cleanup
maierlars
approved these changes
Mar 18, 2020
|
Jenkins blue now |
ObiWahn
added a commit
that referenced
this pull request
Mar 27, 2020
…ql-functions * origin/devel: (25 commits) Bug fix/fixes 20200318 (#11319) updated CHANGELOG Feature/out of search in range (#11324) fix "fix" for collection figures (#11323) updated CHANGELOG compilation fixes for clang-10s more strict checking (#11316) fix failing query (#11317) KShortestPathsExecutor must reset its KShortestPathFinder, including all caches. (#11312) Feature/aql subquery execution block impl execute implementation expected number of rows (#11274) Add DTRACE points to measure request timings. (#11245) USE_STRICT_OPENSSL is Off by default Fix usesRevisionAsDocumentId population and add syncByRevision flag (#11314) Traversal Bugfix (#11310) Bug fix/issue 11275 (#11299) added simple test (#11301) Fix some typos (#10173) Documentation/typos 2020-01-24 (#10975) Update CHANGELOG Some cleanup for new executor test code, which accidentally fixes ASAN failures in ExecutorTestHelper (#11283) LZ4 update (#11306) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR escalated a bit, because while I was fixing a memory leak, it turned out that the Executor tests had multiple unrelated
AqlItemBlockManagerinstances around which removed each other's dependencies blocks, leading to crashes.To fix this, the
ExecutorTestHelpernow gets anAqlItemBlockMangeron construction, and the wholePipelineAPI is gone and hidden inside the test helper.I also deleted a lot of unused test code, in particular the
SubqueryExecutortest, which did nothing. Lets not pretend there are tests for things that aren't there.