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

[timotheecour]

• 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]

Ping @FedericoCeratto since he wrote these tests.

[timotheecour]

ping @FedericoCeratto

[timotheecour]

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.

[timotheecour]

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]

@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]

• 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]

@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).

[alaviss]

And please document the flags you added.

[timotheecour]

And please document the flags you added.

done

[timotheecour]

PTAL

[timotheecour]

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]

Why did you change the mode here?

[timotheecour]

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

[timotheecour]

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,