libc: time.h: add test groups for gmtime, mktime, strftime #255

jmaksymowicz · 2023-09-22T15:14:51Z

Moved tests for time functions (gmtime, mktime and strftime) that existed previously and added them to libc automatic testing.

Description

Tests for the mentioned time functions existed, but were never compiled or checked. The tests for gmtime and mktime functions pass. Some tests for strftime would fail (because not all formats are implemented, or because padding is not implemented), so I set them to be ignored for now.

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: ia32-generic-qemu, armv7a9-zynq7000-zturn

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

github-actions · 2023-09-22T15:29:16Z

Unit Test Results

5 948 tests +64 5 291 ✔️ +50 38m 3s ⏱️ + 1m 8s
  324 suites +  8   657 💤 +14
      1 files ±  0       0 ❌ ±  0

Results for commit 31f95c9. ± Comparison against base commit 21e66a3.

♻️ This comment has been updated with latest results.

nalajcie

Great that you've moved the obsolete tests to unity framework!

I've put some nitpicks found by reading the code (I didn't run it).

nalajcie · 2023-09-25T09:59:36Z

libc/time/gmtime.c

@@ -16,7 +16,9 @@
 #include <time.h>
 #include <stdio.h>
 #include <stdlib.h>
-#include "test_common.h"
+#include "../../test_common.h"


please don't include files from outside of libc subdir - move necessary functions to the common part of libc tests (or to time subfolder if you don't think they would be shared between libc tests submodules)

OK, I made a new header file with common functions for time tests.

nalajcie · 2023-09-25T10:00:56Z

libc/time/gmtime.c

-	{
-		return 0;
-	}
+	TEST_ASSERT_FALSE_MESSAGE(compare_tm(t, &t_exp), "Incorrect struct tm");


maybe it would be better to provide TEST_ASSERT_* attributed compare_tm?

nalajcie · 2023-09-25T10:08:25Z

libc/time/gmtime.c

 {
 	struct tm *t, t_exp;
-	char buff[120];
 	init_tm(&t_exp, output);


probably we could directly keep struct tm in output tab, but you can keep it as is (as it's not directly related to your changes)

nalajcie · 2023-09-25T10:09:08Z

libc/time/host_generation.c

+#include <stdio.h>
+#include <stdlib.h>
+
+#include "../../test_common.h"


don't include files from outside of libc subdir

❗ Please note that this file introduces new compilation warnings

nalajcie · 2023-09-25T10:10:15Z

libc/time/mktime.c

@@ -16,7 +16,9 @@
 #include <time.h>
 #include <stdio.h>
 #include <stdlib.h>
-#include "test_common.h"
+#include "../../test_common.h"


don't include files from outside of libc subdir

nalajcie · 2023-09-25T15:42:45Z

libc/time/main.c

+	UnityMain(argc, (const char **)argv, runner);
+	return 0;


I know this is ^C^V from other files but maybe:

Suggested change

UnityMain(argc, (const char **)argv, runner);

return 0;

int failures = UnityMain(argc, (const char **)argv, runner);

return (failures == 0) : 0 : 1;

nalajcie · 2023-09-25T15:43:25Z

libc/time/mktime.c

-	}
+	timestamp = function_under_test(&t);
+	TEST_ASSERT_EQUAL_INT_MESSAGE(timestamp_exp, timestamp, "Incorrect time_t");
+	TEST_ASSERT_FALSE_MESSAGE(compare_tm(&t, &t_exp), "Incorrect struct tm");


TEST_ASSERT_* attributed compare_tm ?

nalajcie · 2023-09-25T15:48:19Z

libc/time/strftime.c

-	printf("STRFTIME TEST STARTED\n");
-	save_env();
+#ifdef __phoenix__
+	TEST_IGNORE();


maybe use TEST_IGNORE_MESSAGE("reason, eg. issue ID")

why these tests are ignored?

I added messages to tests that are ignored for now. The issue is phoenix-rtos/phoenix-rtos-project#351.

nalajcie · 2023-09-25T15:49:20Z

libc/time/strftime.c

+#endif
+#ifndef __phoenix__


Suggested change

#endif

#ifndef __phoenix__

#else

Or don't put it in else (the code after TEST_IGNORE won't run anyway)

nalajcie · 2023-09-26T14:32:33Z

damianloew

Thanks for adapting this piece of code to our environment. The improvements looks clean, small remarks left.

damianloew · 2023-10-05T14:46:15Z

libc/time/gmtime.c

+	RUN_TEST_CASE(time_gmtime, basic_test);
+}
+
+
 static const time_t input_vector[] = {
 	/* 29 FEB 2016 - leap year */
 	61414866000,


The values tested are in fact 1900 years further:

I know that it's not your code and those values are not so crucial, but it would be nice to correct them before enabling in CI (they may be misleading).

Same for mktime tests.

damianloew · 2023-10-05T17:18:04Z

libc/time/mktime.c

 }


-int mktime_assert(const int input[], const int output[], const time_t timestamp_exp)
+static void mktime_assert(const int input[], const int output[], const time_t timestamp_exp)
 {
 	struct tm t, t_old, t_exp;


t_old is not needed.

damianloew · 2023-10-05T17:25:44Z

libc/time/host_generation.c

+			.tm_hour = rand() % 23,
+			.tm_mday = rand() % 30,
+			.tm_mon = rand() % 11,
+			.tm_year = 1980 + rand() % 60,


There is an issue mentioned above.

Suggested change

.tm_year = 1980 + rand() % 60,

.tm_year = 80 + rand() % 60,

damianloew · 2023-10-05T17:39:00Z

libc/time/strftime.c


-static const struct test_data test_vector[T1_LEN] = {
+static const struct test_data basic_formatting[] = {


I think that we could place the declarations at the beginning of the file
like it's done in the rest of time tests. So sth like:

Suggested change

static const struct test_data basic_formatting[] = {

static const struct test_data basic_formatting[T1_LEN];

static const struct test_data additional_format_chars[T2_LEN];

...

static const struct test_data basic_formatting[] = {

Then we could place test data below TEST_GROUP_RUNNER.

Alternatively you could switch to the convention applied in this test in mktime/strftime tests - we just want to be consistent.

github-actions · 2023-10-12T14:18:40Z

libc/time/gmtime.c

+#include "time_common.h"
+
+
+#define NCOLS 9


[clang-format-pr] _{reported by reviewdog 🐶}
suggested fix

Suggested change

#define NCOLS 9

#define NCOLS 9

damianloew

Thanks for applying the suggestions, please rebase changes and we can merge it :)

JIRA: RTOS-609

damianloew

LGTM

jmaksymowicz requested review from agkaminski and damianloew September 22, 2023 15:14

jmaksymowicz force-pushed the libc_time_tests branch from 8453adc to 54db782 Compare September 22, 2023 15:16

jmaksymowicz force-pushed the libc_time_tests branch from 54db782 to b2089c0 Compare September 25, 2023 11:14

jmaksymowicz mentioned this pull request Sep 25, 2023

Fix bug in formatting timestamps with gmtime_r phoenix-rtos/libphoenix#293

Merged

13 tasks

nalajcie reviewed Sep 25, 2023

View reviewed changes

jmaksymowicz force-pushed the libc_time_tests branch 2 times, most recently from 52b78d5 to 2281a4e Compare September 26, 2023 13:34

jmaksymowicz force-pushed the libc_time_tests branch from 2281a4e to 78017e5 Compare September 26, 2023 14:45

damianloew requested changes Oct 5, 2023

View reviewed changes

jmaksymowicz mentioned this pull request Oct 12, 2023

mktime invalid normalization phoenix-rtos/libphoenix#28

Open

jmaksymowicz force-pushed the libc_time_tests branch from 78017e5 to 72220c4 Compare October 12, 2023 14:18

github-actions bot reviewed Oct 12, 2023

View reviewed changes

jmaksymowicz force-pushed the libc_time_tests branch from 72220c4 to e71156f Compare October 12, 2023 14:24

damianloew reviewed Oct 18, 2023

View reviewed changes

libc: time.h: add test groups for gmtime, mktime, strftime

31f95c9

JIRA: RTOS-609

jmaksymowicz force-pushed the libc_time_tests branch from e71156f to 31f95c9 Compare October 18, 2023 17:02

damianloew approved these changes Oct 19, 2023

View reviewed changes

damianloew merged commit 351d060 into master Oct 19, 2023
30 checks passed

damianloew deleted the libc_time_tests branch October 19, 2023 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libc: time.h: add test groups for gmtime, mktime, strftime #255

libc: time.h: add test groups for gmtime, mktime, strftime #255

jmaksymowicz commented Sep 22, 2023

github-actions bot commented Sep 22, 2023 •

edited

Loading

nalajcie left a comment

nalajcie Sep 25, 2023

jmaksymowicz Sep 26, 2023

nalajcie Sep 25, 2023

jmaksymowicz Sep 26, 2023

nalajcie Sep 25, 2023

nalajcie Sep 25, 2023

nalajcie Sep 25, 2023

nalajcie Sep 25, 2023

nalajcie Sep 25, 2023

nalajcie Sep 25, 2023

jmaksymowicz Sep 26, 2023

nalajcie Sep 25, 2023

nalajcie commented Sep 26, 2023

damianloew left a comment

damianloew Oct 5, 2023

damianloew Oct 5, 2023

damianloew Oct 5, 2023

damianloew Oct 5, 2023

damianloew Oct 5, 2023

damianloew Oct 5, 2023

damianloew Oct 5, 2023

damianloew Oct 5, 2023

github-actions bot Oct 12, 2023

damianloew left a comment

damianloew left a comment


		static const struct test_data test_vector[T1_LEN] = {
		static const struct test_data basic_formatting[] = {

libc: time.h: add test groups for gmtime, mktime, strftime #255

libc: time.h: add test groups for gmtime, mktime, strftime #255

Conversation

jmaksymowicz commented Sep 22, 2023

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

github-actions bot commented Sep 22, 2023 • edited Loading

Unit Test Results

nalajcie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalajcie commented Sep 26, 2023

damianloew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Oct 12, 2023

Choose a reason for hiding this comment

damianloew left a comment

Choose a reason for hiding this comment

damianloew left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 22, 2023 •

edited

Loading