-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
cpp-qt5-client: add valgrind memory test #3663
Conversation
I think it is a good approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little concern regarding debug -O0 flag, otherwise seems fine to me
@@ -26,3 +26,6 @@ SOURCES += main.cpp \ | |||
HEADERS += PetApiTests.h \ | |||
StoreApiTests.h \ | |||
UserApiTests.h | |||
|
|||
# Disable optimisation for better valgrind report | |||
QMAKE_CXXFLAGS += -O0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about QMAKE_CXXFLAGS_DEBUG
?
in build-and-test.bash
you call qmake
with CONFIG+=debug
, so I assume debug flags will be used (I might be wrong though, I've never used qmake
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
5df62cf
to
506f840
Compare
There is still memory leak. After investigation, it seems that the Shouldn't we first merge #3667 which remove this instruction? |
506f840
to
6f7030c
Compare
I restarted CI and it passed . |
Intermittent CI tests failure due to
Ref: https://travis-ci.org/OpenAPITools/openapi-generator/builds/576220434 Please take a look when you've time. Have a nice weekend. |
* master: (45 commits) update python samples Two tiny fixes for Java Vertx client (#3683) Exclude "test" from packages so that it is not installed as top-level package when "pip installing" the generated sdk (#3731) [java-client] add unit test for JDK 11 HttpClient (#3735) [R] feat(r): handling error response for 3xx cases (#3571) [Dart] Avoid redundant null checking in fromJson (#3632) [core] Set isMultipart=true for multipart operations, fix possible NPE (#3750) reenable kotlin client tests in shippable ci (#3751) Fix null assignation to header/query param map (#3749) Mockito 3.0.0 (#3732) Update to swagger-parser to 2.0.14-OpenAPITools.org-1 (#3737) Add a link to a Qiita blog post (#3747) update csharp samples Simple spelling error in dotnet core comments (#3746) better support for multipart (#3736) [typescript-fetch] fix #1998: prefix req ifaces. (#3740) cpp-qt5-client: add valgrind memory test (#3663) [typescript-angular] Encode objects for multipart/form-data with json - fixes #2733 (#3738) [BUG] Https protocol schema is not handled correctly in java 11 native code #3729 (#3730) comment out kotlin client tests ...
Even if the test passed, I still found
Ref: https://travis-ci.org/OpenAPITools/openapi-generator/builds/576585660 |
Commented out qt5 c++ tests via 7e772e1 as we plan to release 4.1.1 |
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
,./bin/openapi3/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.master
,4.1.x
,5.0.x
. Default:master
.Description of the PR
This PR perfoms valgrind test for Qt5 client test. #3661 is not merge so it should fail (I'll rebase once it is merged).
What do you think of the way I handle valgrind result?
@ravinikam @stkrwork @etherealjoy @muttleyxd