remove .github/workflows/ci_ssl.yml; instead run via trunner_thirdparty #16221

timotheecour · 2020-12-01T20:57:02Z

simplifies testing logic (removing duplication in yml files)
reduces CI testing time (most of which is spent here on simply bootstrapping nim)
allows running ssl tests on each push/PR, avoiding cases where a regression is only found a (potentially) long time after it's introduced
closes [superseded] test ci_ssl.yml on each PR/push #16220

update

remove noop --pedantic
add --putenv:NIM_TESTAMENT_REMOTE_NETWORKING:1 in runCI which gets propagated all the way through process calls down to individual tests (easiest way to propagate a flag) so that by default, CI will run with external networking enabled, and running locally testament will not; but can be easily overridden with --putenv:NIM_TESTAMENT_REMOTE_NETWORKING:1. This is abstracted via testutils.enableRemoteNetworking so individual tests can query for this in a typesafe way
simplified related code in tests/misc/trunner.nim
mv /{thttpclient_ssl.nim => thttpclient_ssl_remotenetwork.nim} to avoid name clash
add -d:nimTestsEnableFlaky to ease identifying tests broken on a specific platform and make ie easy to enable those tests locally by changing a single flag (in tests/config.nims)

note: use hide whitespace changes to view a smaller diff (because of re-indentation i needed)

future work

all tests in tests/untestable/* should also be run in trunner_thirdparty, possibly by adjusting constants affecting their running time

alaviss · 2020-12-02T04:35:05Z

Ping @FedericoCeratto since he wrote these tests.

timotheecour · 2020-12-05T21:07:51Z

ping @FedericoCeratto

timotheecour · 2020-12-10T23:55:54Z

ping @FedericoCeratto

FedericoCeratto

These are integration tests. How about creating a tests/integration directory with a dedicated README, to collect such tests?

Also, the PR remove the github action without adding a new one it seems.
If the new file is picked up automatically by testament this becomes a problem: people should be able to run the "default" test suite without any network interaction.

tests/misc/trunner_thirdparty.nim

timotheecour · 2020-12-11T19:16:49Z

These are integration tests. How about creating a tests/integration directory with a dedicated README, to collect such tests?

That's not specific to that test, tests/dll/nimhcr_integration.nim (and others) also is an integration test but is rooted under its category (dll), not under some tests/integration. Plus, the boundary between integration and unit is blurry in practice.

Here's a better idea for future work: testament spec tags, see timotheecour#445.

As explained in this RFC, tags, unlike file hierarchies, can model fact that a given test can be associated with different tags, eg: integration, ssl, networking etc.

Also, the PR remove the github action without adding a new one it seems.

That's the whole point, as explained in PR, a separate github action for this test isnt' needed, the test runs via existing CI pipelines (via tests/misc/trunner_thirdparty.nim) without overhead of having to maintain a separate github action pipeline (not DRY), as well as being more resource efficient (doesnt' need to re-bootstrap nim), and allows it to run on every PR/push.

If the new file is picked up automatically by testament this becomes a problem: people should be able to run the "default" test suite without any network interaction.

that's a good point; timotheecour#445 would solve this nicely via:

testament --tags:"-networking" all # all test minus ones that contain networking tag
testament --tags:"benchmarking,-networking" all # all tests that contain tag benchmarking but not networking

--tags can have some default value (eg "-networking") so that by default we run networking tests in CI but not locally.

In the meantime, I can also simply add a custom define flag in koch.runCI to address this specific point.

timotheecour · 2020-12-12T02:58:48Z

@FedericoCeratto PTAL, see update in top PR msg, in particular it addresses your last remaining concern:

If the new file is picked up automatically by testament this becomes a problem: people should be able to run the "default" test suite without any network interaction.

alaviss · 2020-12-12T06:12:22Z

add --putenv:NIM_TESTAMENT_REMOTE_NETWORKING:1 in runCI which gets propagated all the way through process calls down to individual tests (easiest way to propagate a flag) so that by default, CI will run with external networking enabled, and running locally testament will not; but can be easily overridden with --putenv:NIM_TESTAMENT_REMOTE_NETWORKING:1. This is abstracted via testutils.enableRemoteNetworking so individual tests can query for this in a typesafe way

An actual flag to testament would be appreciated.

timotheecour · 2020-12-12T06:35:53Z

@alaviss

An actual flag to testament would be appreciated.

I thought about this but decided it'd be better to use --putenv:NIM_TESTAMENT_REMOTE_NETWORKING for now for 2 reasons:

env vars propagate to children (recursively), so this guarantees CI will run the corresponding tests, without having to add lots of complexity in testament: the alternative is to add custom logic just to propagate some flag along this process call chain:
runCI => testament all => testament misc => trunner_special => {thttpclient_ssl.nim, thttpclient_ssl_disabled.nim}

so that runCI runs with networking and running locally runs without networking.
instead of adding a custom testament flag just for enabling/disabling networking, we can generalize via test tags (see testament: use spec tags (more flexible than file hierarchies / dir category) timotheecour/Nim#445). Once we get to that in future work, we can remove --putenv:NIM_TESTAMENT_REMOTE_NETWORKING and use tags instead:

testament --tags:benchmarking,js,-networking` # runs all js benchmarking tests except ones with networking

which avoids adding N flags and instead simplifies into 1 flag (--tags) and N tags (js, networking, benchmarking, flaky, etc) that can be used for filtering what to run (generalizes testament categories which are hardcoded to a single dir and can't express fact that some tests can be associated with multiple tags).

testament/lib/stdtest/testutils.nim

alaviss · 2020-12-12T06:58:02Z

And please document the flags you added.

timotheecour · 2020-12-12T07:45:32Z

And please document the flags you added.

done

timotheecour · 2020-12-13T02:00:43Z

PTAL

timotheecour · 2021-01-14T01:11:21Z

ping @alaviss (had to rebase again for conflicts)

is this good to go?

this is needed to avoid things like #16667 (comment)

alaviss

testament looks good to me

alaviss · 2021-01-14T01:54:45Z

tests/untestable/thttpclient_ssl_disabled.nim

-    var client = newHttpClient(sslContext = ctx)
-    let a = $client.getContent(expired)
+    test "httpclient in insecure mode":
+      var ctx = newContext(verifyMode = CVerifyPeerUseEnvVars)


Why did you change the mode here?

I didn't, it's a github diff artifact. check the diff more carefully (look also with "ignore whitespace", or with a local git diff)

alaviss

LGTM

…oken tests and enable them locally

timotheecour · 2021-02-02T08:50:17Z

PTAL, I had to rebase again to avoid bitrot conflicts.

note: the diff is smaller than it seems from github; eg:

git diff -w origin/devel:tests/untestable/thttpclient_ssl.nim HEAD:tests/untestable/thttpclient_ssl_remotenetwork.nim

diff --git a/tests/untestable/thttpclient_ssl.nim b/tests/untestable/thttpclient_ssl_remotenetwork.nim
index 038eba99d..ed6866d53 100644
--- a/tests/untestable/thttpclient_ssl.nim
+++ b/tests/untestable/thttpclient_ssl_remotenetwork.nim
@@ -6,13 +6,15 @@
 #    See the file "copying.txt", included in this
 #    distribution, for details about the copyright.
 #
-## Warning: this test performs external networking.
 ## Test with:
-## ./bin/nim c -d:ssl -p:. --threads:on -r tests/untestable/thttpclient_ssl.nim
+## nim r --putenv:NIM_TESTAMENT_REMOTE_NETWORKING:1 -d:ssl -p:. --threads:on tests/untestable/thttpclient_ssl_remotenetwork.nim
 ##
 ## See https://github.com/FedericoCeratto/ssl-comparison/blob/master/README.md
 ## for a comparison with other clients.

+from stdtest/testutils import enableRemoteNetworking
+when enableRemoteNetworking and (defined(nimTestsEnableFlaky) or not defined(windows) and not defined(openbsd) and not defined(i386)):
+  # Not supported on Windows due to old openssl version
   import
     httpclient,
     net,

…ty (nim-lang#16221)

timotheecour mentioned this pull request Dec 1, 2020

[superseded] test ci_ssl.yml on each PR/push #16220

Closed

timotheecour force-pushed the pr_remove_ci_ssl branch from 76c4cd4 to 874aae0 Compare December 1, 2020 21:49

timotheecour mentioned this pull request Dec 1, 2020

flaky test: windows: tests/gc/weakrefs c -d:release -d:useRealtimeGC; tests/gc/thavlak; tests/gc/gcleak4 c --gc:markAndSweep timotheecour/Nim#411

Open

timotheecour marked this pull request as ready for review December 1, 2020 22:40

timotheecour mentioned this pull request Dec 4, 2020

fix partially #13115 (now works for cpp; but still fails for js on openbsd) #16167

Merged

1 task

FedericoCeratto requested changes Dec 11, 2020

View reviewed changes

tests/misc/trunner_thirdparty.nim Outdated Show resolved Hide resolved

timotheecour force-pushed the pr_remove_ci_ssl branch from 874aae0 to ab7d984 Compare December 11, 2020 19:20

alaviss reviewed Dec 12, 2020

View reviewed changes

testament/lib/stdtest/testutils.nim Outdated Show resolved Hide resolved

This was referenced Dec 12, 2020

thttpclient_ssl flaky test on Linux_i386 timotheecour/Nim#448

Open

thttpclient_ssl.nim, thttpclient_ssl_remotenetwork.nim fails on linux: genericDeepCopyAux SIGSEGV #16338

Open

timotheecour force-pushed the pr_remove_ci_ssl branch from f54fd02 to 96ceca0 Compare December 13, 2020 01:59

timotheecour force-pushed the pr_remove_ci_ssl branch from 96ceca0 to f3e62bf Compare December 13, 2020 20:52

timotheecour force-pushed the pr_remove_ci_ssl branch from f3e62bf to ef17fcf Compare January 13, 2021 22:30

timotheecour mentioned this pull request Jan 13, 2021

A new request should always have a new content-length #16667

Merged

alaviss reviewed Jan 14, 2021

View reviewed changes

alaviss approved these changes Jan 14, 2021

View reviewed changes

timotheecour force-pushed the pr_remove_ci_ssl branch from ef17fcf to 6229500 Compare February 2, 2021 04:27

timotheecour added 2 commits February 1, 2021 23:29

remove .github/workflows/ci_ssl.yml; instead run via trunner_thirdparty

ab44829

fixup

055b92b

timotheecour added 10 commits February 1, 2021 23:29

fixup

4aa2bba

fixup

12a5b1c

trunner_thirdparty => trunner_special

1cdfe09

NIM_TESTAMENT_REMOTE_NETWORKING

b25e555

fixup

86067c4

document NIM_TESTAMENT_REMOTE_NETWORKING

404c4b2

fixup

84e335f

address comment

22f6f21

introduce defined(nimTestsEnableFlaky) to make it easy to identify br…

bfe53d1

…oken tests and enable them locally

fixup

7ed5726

timotheecour force-pushed the pr_remove_ci_ssl branch from 6229500 to 7ed5726 Compare February 2, 2021 07:30

fixup

f0a2b0a

Araq closed this Feb 2, 2021

Araq reopened this Feb 2, 2021

ringabout approved these changes Feb 3, 2021

View reviewed changes

timotheecour merged commit 6f1289b into nim-lang:devel Feb 3, 2021

timotheecour deleted the pr_remove_ci_ssl branch February 3, 2021 02:32

ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021

remove .github/workflows/ci_ssl.yml; instead run via trunner_thirdpar…

482ceb4

…ty (nim-lang#16221)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove .github/workflows/ci_ssl.yml; instead run via trunner_thirdparty #16221

remove .github/workflows/ci_ssl.yml; instead run via trunner_thirdparty #16221

timotheecour commented Dec 1, 2020 •

edited

Loading

alaviss commented Dec 2, 2020

timotheecour commented Dec 5, 2020

timotheecour commented Dec 10, 2020

FedericoCeratto left a comment •

edited

Loading

timotheecour commented Dec 11, 2020

timotheecour commented Dec 12, 2020 •

edited

Loading

alaviss commented Dec 12, 2020 •

edited

Loading

timotheecour commented Dec 12, 2020 •

edited

Loading

alaviss commented Dec 12, 2020

timotheecour commented Dec 12, 2020

timotheecour commented Dec 13, 2020

timotheecour commented Jan 14, 2021

alaviss left a comment

alaviss Jan 14, 2021

timotheecour Jan 14, 2021 •

edited

Loading

alaviss left a comment

timotheecour commented Feb 2, 2021 •

edited

Loading

remove .github/workflows/ci_ssl.yml; instead run via trunner_thirdparty #16221

remove .github/workflows/ci_ssl.yml; instead run via trunner_thirdparty #16221

Conversation

timotheecour commented Dec 1, 2020 • edited Loading

update

future work

alaviss commented Dec 2, 2020

timotheecour commented Dec 5, 2020

timotheecour commented Dec 10, 2020

FedericoCeratto left a comment • edited Loading

Choose a reason for hiding this comment

timotheecour commented Dec 11, 2020

timotheecour commented Dec 12, 2020 • edited Loading

alaviss commented Dec 12, 2020 • edited Loading

timotheecour commented Dec 12, 2020 • edited Loading

alaviss commented Dec 12, 2020

timotheecour commented Dec 12, 2020

timotheecour commented Dec 13, 2020

timotheecour commented Jan 14, 2021

alaviss left a comment

Choose a reason for hiding this comment

alaviss Jan 14, 2021

Choose a reason for hiding this comment

timotheecour Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

alaviss left a comment

Choose a reason for hiding this comment

timotheecour commented Feb 2, 2021 • edited Loading

timotheecour commented Dec 1, 2020 •

edited

Loading

FedericoCeratto left a comment •

edited

Loading

timotheecour commented Dec 12, 2020 •

edited

Loading

alaviss commented Dec 12, 2020 •

edited

Loading

timotheecour commented Dec 12, 2020 •

edited

Loading

timotheecour Jan 14, 2021 •

edited

Loading

timotheecour commented Feb 2, 2021 •

edited

Loading