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

[PATCH v12] one more try to add nice stats for Shippable #309

Closed
wants to merge 5 commits into from

Conversation

muvarov
Copy link
Contributor

@muvarov muvarov commented Nov 24, 2017

No description provided.

@muvarov muvarov changed the title one more try to add nice stats for Shippable [PATCH v1] one more try to add nice stats for Shippable Nov 24, 2017
@muvarov muvarov changed the title [PATCH v1] one more try to add nice stats for Shippable [PATCH v2] one more try to add nice stats for Shippable Nov 24, 2017
#include <odp_api.h>
#include "odp_cunit_common.h"
#include <odp/helper/odph_api.h>
/* Globals */
static odph_odpthread_t thread_tbl[MAX_WORKERS];
static odp_instance_t instance;

extern const char *__progname;
Copy link

Choose a reason for hiding this comment

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

Please use a copy of argv[0] instead of progname.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

save argv[0] in some global variable?

Copy link

Choose a reason for hiding this comment

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

@muvarov yes

}

CU_automated_run_tests();
#else
Copy link

Choose a reason for hiding this comment

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

This should be refactored to a function which is selected basing on TEST_CUNIT_XML, rather than having #ifdef directly in the function body.

Copy link

Choose a reason for hiding this comment

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

@muvarov I still do not like this piece. Maybe you can add a runtime switch through environment variable? Then there will be no requirement for extra AM_CONDITIONAL/AC_DEFINE

AC_ARG_ENABLE([cunit_out_xml],
[AS_HELP_STRING([--enable-cunit_out_xml],
[include additional debugging code ]
[(set to 'full' to enable all --enable-*-debug-* options)])],
Copy link

Choose a reason for hiding this comment

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

This is just incorrect

Copy link
Contributor Author

@muvarov muvarov Nov 27, 2017

Choose a reason for hiding this comment

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

yep, thanks it's copy paste text

@@ -17,6 +17,9 @@ INCFLAGS = \

AM_CPPFLAGS = $(INCFLAGS)
AM_CFLAGS = $(CUNIT_CFLAGS)
if cunit_out_xml
AM_CFLAGS += -DTEST_CUNIT_XML
endif
Copy link

Choose a reason for hiding this comment

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

This looks like an overkill. I'd suggest to use Autoconf variable rather than Automake conditional here.

@muvarov muvarov changed the title [PATCH v2] one more try to add nice stats for Shippable [PATCH v3] one more try to add nice stats for Shippable Nov 24, 2017
@muvarov muvarov changed the title [PATCH v3] one more try to add nice stats for Shippable [PATCH v4] one more try to add nice stats for Shippable Nov 27, 2017
@muvarov
Copy link
Contributor Author

muvarov commented Nov 27, 2017

@ttrahan this PR adds Junit stats and copies it to results directory. But I think parsing script needs to be adjusted to support that output. I do not see that stats are updated.

@muvarov muvarov changed the title [PATCH v4] one more try to add nice stats for Shippable [PATCH v5] one more try to add nice stats for Shippable Dec 7, 2017
@muvarov muvarov changed the title [PATCH v5] one more try to add nice stats for Shippable [PATCH v6] one more try to add nice stats for Shippable Dec 8, 2017
[AS_HELP_STRING([--enable-cunit_out_xml],
[output to xml]
[(output test results to xml instead of plain text)])],
cunit_out_xml=yes, cunit_out_xml=no)
Copy link

Choose a reason for hiding this comment

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

Ideally this should also be escaped with square brackets.

}

CU_automated_run_tests();
#else
Copy link

Choose a reason for hiding this comment

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

@muvarov I still do not like this piece. Maybe you can add a runtime switch through environment variable? Then there will be no requirement for extra AM_CONDITIONAL/AC_DEFINE

#include <CUnit/TestDB.h>
#if TEST_CUNIT_XML
#include <CUnit/Automated.h>
#endif
Copy link

Choose a reason for hiding this comment

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

No need to hide it under #if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

our arm tests use some previous version where function declared as finc(); i.e. not func(void) and gcc fails on it. In task which run inside Shippable it's corrected.

Copy link

Choose a reason for hiding this comment

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

@muvarov then it's better to update CUnit version on target, rather than workaround its bugs.

[TEST_CUNIT_XML=0])
AC_DEFINE_UNQUOTED([TEST_CUNIT_XML], [$TEST_CUNIT_XML],
[Define to 1 to output to xml])
AM_CONDITIONAL([cunit_out_xml], [test "x$cunit_out_xml" = "xyes"])
Copy link

Choose a reason for hiding this comment

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

Conditional is unused

@muvarov muvarov changed the title [PATCH v9] one more try to add nice stats for Shippable [PATCH v10] one more try to add nice stats for Shippable Dec 14, 2017
@muvarov
Copy link
Contributor Author

muvarov commented Dec 14, 2017

@lumag fixed comments. This version has to show counters. For Cunit I think we only can fix header because it's already the latest release. I added comment to travis. Please take a look.

@muvarov
Copy link
Contributor Author

muvarov commented Dec 14, 2017

Travis fails here due to caches for cunit. After merge it has to be ok. Clean run is here:
https://travis-ci.org/muvarov/odp/builds/316450770

@muvarov
Copy link
Contributor Author

muvarov commented Dec 19, 2017

@lumag ping.

@lumag
Copy link

lumag commented Dec 19, 2017

@muvarov still an #ifdef inside function code. I really suggest to move xml/plain output type to test arguments.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
Pass args to cunit to make code commong with other tests.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
@muvarov muvarov changed the title [PATCH v10] one more try to add nice stats for Shippable [PATCH v11] one more try to add nice stats for Shippable Dec 20, 2017
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
some gcc fails on including this header but cunit is the latest stable
release. So fix it in place until it will not be fixed upstream.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
@muvarov muvarov changed the title [PATCH v11] one more try to add nice stats for Shippable [PATCH v12] one more try to add nice stats for Shippable Dec 20, 2017
@muvarov
Copy link
Contributor Author

muvarov commented Dec 20, 2017

v12: switched to getenv() instead of ifdefs as agreed.

@lumag
Copy link

lumag commented Dec 20, 2017

Reviewed-by: Dmitry Eremin-Solenikov

@muvarov
Copy link
Contributor Author

muvarov commented Dec 21, 2017

Merged.

@muvarov muvarov closed this Dec 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants