[tests] modify sync cases in timestamp test

[KFilipek]

Related to #211.
Depends on: pmem/miniasync#109

This change is 

[codecov-commenter]

Codecov Report

Merging #248 (b2be501) into master (13aa782) will decrease coverage by 1.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
- Coverage   89.35%   88.27%   -1.08%     
==========================================
  Files           9        9              
  Lines         742      776      +34     
  Branches      134      142       +8     
==========================================
+ Hits          663      685      +22     
- Misses         47       56       +9     
- Partials       32       35       +3     

Flag	Coverage Δ
tests_gcc_debug_cpp17	88.27% <ø> (-1.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pmemstream/src/libpmemstream.c	86.07% <0.00%> (-1.84%)	⬇️
pmemstream/src/region.c	89.06% <0.00%> (ø)
pmemstream/src/libpmemstream_internal.h	100.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[igchor]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @karczex, @KFilipek, and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 14 at r1 (raw file):

 */

void multithreaded_asynchronous_append(pmemstream_test_base &stream, const std::vector<pmemstream_region> &regions,

where is the multithreading in this test?

---

tests/unittest/timestamp.cpp line 98 at r1 (raw file):

		// XXX: implement asynchronous cases
		ret += rc::check("timestamp values should increase in each region after asynchronous append",

All those tests look exactly the same as the sync ones, Can't you just make them generic? (e.g. make a template and pass append function as a parameter or just accept a bool param in lambda and decide whether to do sync or async based on it?)

[lukaszstolarczuk]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @karczex and @KFilipek)

---

-- commits line 2 at r1:
async

---

tests/unittest/timestamp.cpp line 20 at r1 (raw file):

	std::vector<future_type> futures;
	for (size_t i = 0; i < regions.size(); i++) {
		futures.emplace_back(stream.helpers.async_append(regions[i], data));

do we poll these futures somewhere?

---

tests/unittest/timestamp.cpp line 39 at r1 (raw file):

	struct test_config_type test_config;
	test_config.filename = std::string(argv[1]);

I think we should use config's max_concurrency, because CI timed-out on pmemcheck test (probably because we had too many regions...?)

---

tests/unittest/timestamp.cpp line 97 at r1 (raw file):

			});

		// XXX: implement asynchronous cases

I guess you can get rid of this comment, after this PR 😉

[lukaszstolarczuk]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @karczex and @KFilipek)

---

tests/unittest/timestamp.cpp line 20 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

do we poll these futures somewhere?

as discussed it should be polled somewhere within the test body, not in the dtor (of future wrapper)

---

tests/unittest/timestamp.cpp line 88 at r1 (raw file):

				/* Global ordering validation. */
				if (regions.size() > 1)

are you rebased to the latest master? I think this value line is updated on the master branch 😉

[igchor]

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @karczex, @KFilipek, and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 22 at r4 (raw file):

	std::vector<std::vector<future_type>> futures(concurrency_level);

	for (size_t i = 0; i < concurrency_level; i++) {

Didn't we want to test multithreaded appends? Why it's not inside parallel_exec?

[KFilipek]

Reviewed 1 of 1 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @igchor, @karczex, @KFilipek, and @lukaszstolarczuk)

---

-- commits line 2 at r1:

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

async

Done.

---

tests/unittest/timestamp.cpp line 14 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

where is the multithreading in this test?

Right

---

tests/unittest/timestamp.cpp line 20 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

as discussed it should be polled somewhere within the test body, not in the dtor (of future wrapper)

Done.

---

tests/unittest/timestamp.cpp line 39 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I think we should use config's max_concurrency, because CI timed-out on pmemcheck test (probably because we had too many regions...?)

Done.

---

tests/unittest/timestamp.cpp line 88 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

are you rebased to the latest master? I think this value line is updated on the master branch 😉

Done.

---

tests/unittest/timestamp.cpp line 97 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I guess you can get rid of this comment, after this PR 😉

Done.

[lukaszstolarczuk]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @igchor, @karczex, and @KFilipek)

---

-- commits line 5 at r5:
?

---

tests/unittest/timestamp.cpp line 98 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

All those tests look exactly the same as the sync ones, Can't you just make them generic? (e.g. make a template and pass append function as a parameter or just accept a bool param in lambda and decide whether to do sync or async based on it?)

what about this one? I think for sync tests it makes a little more sense to use "chunks", but it wouldn't hurt, so this change is still possible :-)

---

tests/unittest/timestamp.cpp line 17 at r5 (raw file):

void multithreaded_asynchronous_append(pmemstream_test_base &stream, const std::vector<pmemstream_region> &regions,
				       const std::vector<std::vector<std::string>> &data, size_t concurrency_level)

concurrency_level == data.size() (?)

---

tests/unittest/timestamp.cpp line 178 at r5 (raw file):

				size_t elements = 0;
				for (auto &part : data) {

pls use chunk name to be consistent with the helper function

[KFilipek]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @igchor, @karczex, and @lukaszstolarczuk)

---

-- commits line 5 at r5:

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

?

Done.

---

tests/unittest/timestamp.cpp line 98 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

what about this one? I think for sync tests it makes a little more sense to use "chunks", but it wouldn't hurt, so this change is still possible :-)

I unified tests a lot, please review this part once again

---

tests/unittest/timestamp.cpp line 22 at r4 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Didn't we want to test multithreaded appends? Why it's not inside parallel_exec?

Done.

---

tests/unittest/timestamp.cpp line 17 at r5 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

concurrency_level == data.size() (?)

it's max number of threads will be run

---

tests/unittest/timestamp.cpp line 178 at r5 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

pls use chunk name to be consistent with the helper function

Done.

[lukaszstolarczuk]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @igchor, @karczex, and @KFilipek)

---

tests/unittest/timestamp.cpp line 17 at r5 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

it's max number of threads will be run

but data is generated using concurrency_level, so I guess this param is redundant here...?

---

tests/unittest/timestamp.cpp line 59 at r7 (raw file):

{
	auto regions = stream.helpers.get_regions();
	size_t concurrency_level = std::min(regions.size(), test_config.max_concurrency);

use get_concurrency_level()

---

tests/unittest/timestamp.cpp line 173 at r7 (raw file):

				RC_PRE(extra_data.size() > 0);
				auto [regions, elements] =
					generate_and_append_data(stream, test_config, true /* sync */);

/* async */

[KFilipek]

Reviewed 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @igchor, @karczex, and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 17 at r5 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

but data is generated using concurrency_level, so I guess this param is redundant here...?

Done. Previously I don't get your point of view, right

---

tests/unittest/timestamp.cpp line 59 at r7 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

use get_concurrency_level()

Done.

---

tests/unittest/timestamp.cpp line 173 at r7 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

/* async */

Done.

[igchor]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @karczex, @KFilipek, and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 22 at r4 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Done.

Still not done? I don't see any appends inside parallel_exec

[lukaszstolarczuk]

Besides Igor's issue

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karczex and @KFilipek)

[KFilipek]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @igchor, @karczex, and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 22 at r4 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Still not done? I don't see any appends inside parallel_exec

Done.

[KFilipek]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk)

a discussion (no related file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

pls use the latest miniasync version in dockers

Done.

---

---

tests/unittest/timestamp.cpp line 28 at r12 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I think we want the opposite of that - from the test scenario point of view I think it'd be nice to do it independently, rather than start the second operation only after the first part is completely done

Done.

---

tests/unittest/timestamp.cpp line 28 at r15 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

as discussed: random seed + tid (*rc::gen::arbitrary<size_t>() + thread_id)

I removed it because of *rc::gen::arbitrary<size_t>() was the problem with waiting on lock (o.O)
Without this part it works like a charm.

[lukaszstolarczuk]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 28 at r12 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Done.

now, I believe syncthreads_barrier is redundant

---

tests/unittest/timestamp.cpp line 28 at r15 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

I removed it because of *rc::gen::arbitrary<size_t>() was the problem with waiting on lock (o.O)
Without this part it works like a charm.

but now there's no randomization here...? thread_id will always be sequential and most likely identical between runs...?

[igchor]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 28 at r15 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

but now there's no randomization here...? thread_id will always be sequential and most likely identical between runs...?

Right, I also don't like this approach. Since miniasync is already fix, please revert those changes (leave 2 separate parallel_execs) and just use correct miniasync version in docker image.

[KFilipek]

Reviewed 2 of 2 files at r18, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 28 at r12 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

now, I believe syncthreads_barrier is redundant

Done.

---

tests/unittest/timestamp.cpp line 28 at r15 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Right, I also don't like this approach. Since miniasync is already fix, please revert those changes (leave 2 separate parallel_execs) and just use correct miniasync version in docker image.

Done.

[lukaszstolarczuk]

Reviewed 1 of 2 files at r18, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @igchor and @KFilipek)

---

tests/unittest/timestamp.cpp line 62 at r18 (raw file):

	size_t concurrency_level = get_concurrency_level(test_config, regions);
	if (regions.size() > concurrency_level) {
		regions.erase(regions.begin() + static_cast<int>(concurrency_level), regions.end());

why is this erasing happening? we generate as many data vectors as concurrency_level (see below generator) - if there are any unused regions it's not a problem

perhaps, we should worry the other way around - what if concurrency level > regions.size()? can this happen?

---

tests/unittest/timestamp.cpp line 104 at r18 (raw file):

	return run_test(test_config, [&] {
		return_check ret;
		test_config = get_test_config();

why do you need this line? test_config is passed to the run_test wrapper

---

tests/unittest/timestamp.cpp line 176 at r18 (raw file):

			[&](pmemstream_with_multi_empty_regions &&stream, const std::vector<std::string> &extra_data) {
				RC_PRE(extra_data.size() > 0);
				RC_PRE(extra_data.size() < 128);

why <128?

[KFilipek]

Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 62 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

why is this erasing happening? we generate as many data vectors as concurrency_level (see below generator) - if there are any unused regions it's not a problem

perhaps, we should worry the other way around - what if concurrency level > regions.size()? can this happen?

It doesn't triggered after line 104.

---

tests/unittest/timestamp.cpp line 104 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

why do you need this line? test_config is passed to the run_test wrapper

I use few times 'test_config' instead of calling get_test_config(). The run_test() creates copy of config object.

---

tests/unittest/timestamp.cpp line 176 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

why <128?

Done. Only for build testing, I've removed it in latest revision.

[lukaszstolarczuk]

Reviewed all commit messages.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @igchor and @KFilipek)

---

tests/unittest/timestamp.cpp line 62 at r18 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

It doesn't triggered after line 104.

I'm not sure what you're saying...

---

tests/unittest/timestamp.cpp line 104 at r18 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

I use few times 'test_config' instead of calling get_test_config(). The run_test() creates copy of config object.

I'm not sure if I understand the problem...

[KFilipek]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 62 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I'm not sure what you're saying...

Done. nvm, it was dead code after adding line 104

---

tests/unittest/timestamp.cpp line 104 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I'm not sure if I understand the problem...

Look at the declaration of run_test()

[lukaszstolarczuk]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @igchor and @KFilipek)

---

tests/unittest/timestamp.cpp line 62 at r18 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Done. nvm, it was dead code after adding line 104

I'm not sure what line 104 (https://github.com/pmem/pmemstream/pull/248/files#diff-9126e2b93f71c2e4ad4cf05036c2e1b8def1558a31a08a2a4fdd808b183b6dc6R104) has to do with it?

And my second question is still unanswered - "what if concurrency level > regions.size()? can this happen?"

---

tests/unittest/timestamp.cpp line 104 at r18 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Look at the declaration of run_test()

I've seen it and still am not sure what the problem is...
so what, we make a copy of the test_config? it still has the proper values - set a few lines above

[lukaszstolarczuk]

Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @igchor and @KFilipek)

a discussion (no related file):
also, CI is failing on timestamp_0_pmemcheck 🤕

---

[lukaszstolarczuk]

Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @igchor and @KFilipek)

---

tests/unittest/timestamp.cpp line 104 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I've seen it and still am not sure what the problem is...
so what, we make a copy of the test_config? it still has the proper values - set a few lines above

as discussed, pls add XXXs here and in run_test

[KFilipek]

Reviewed 1 of 1 files at r21, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)

---

tests/unittest/timestamp.cpp line 62 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I'm not sure what line 104 (https://github.com/pmem/pmemstream/pull/248/files#diff-9126e2b93f71c2e4ad4cf05036c2e1b8def1558a31a08a2a4fdd808b183b6dc6R104) has to do with it?

And my second question is still unanswered - "what if concurrency level > regions.size()? can this happen?"

The code is deleted, so I think it doesn't matter

---

tests/unittest/timestamp.cpp line 104 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

as discussed, pls add XXXs here and in run_test

Done. The change we talked about here was merged some time ago (105f049)

[KFilipek]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)

a discussion (no related file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

also, CI is failing on timestamp_0_pmemcheck 🤕

Done.

---

[lukaszstolarczuk]

Reviewed 1 of 1 files at r21, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @igchor)