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

action: run on windows #3223

Merged
merged 15 commits into from
Mar 31, 2023
Merged

action: run on windows #3223

merged 15 commits into from
Mar 31, 2023

Conversation

v1v
Copy link
Member

@v1v v1v commented Mar 29, 2023

This is one of the very last things to be migrated besides of the benchmarks or releases.

Background

#1393 was the original request that enabled Windows using docker linux containers for running all the required services.

Blockers

Cannot run docker containers on windows. At present docker operations is not supported on Windows runners -> actions/runner#904

Questions

  • Is this particular stage needed?
  • If so, is it needed to run always on PRs? Or nightly could be a good solution?

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Mar 29, 2023
@apmmachine
Copy link
Contributor

apmmachine commented Mar 29, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview previewSnapshots

Expand to view the summary

Build stats

  • Start Time: 2023-03-31T08:52:20.332+0000

  • Duration: 3 min 5 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • run benchmark tests : Run the benchmark test only.

  • run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@trentm
Copy link
Member

trentm commented Mar 29, 2023

  • Is this particular stage needed?

I would like to have some basic testing of the APM agent on Windows, yes.

However, I'd be fine if we skipped any of the test files that require an external service (like redis, postgres, etc.).
I think the way I would do that is:

diff --git a/test/instrumentation/modules/redis.test.js b/test/instrumentation/modules/redis.test.js
index 73d0962e..1fd3d8c8 100644
--- a/test/instrumentation/modules/redis.test.js
+++ b/test/instrumentation/modules/redis.test.js
@@ -9,6 +9,11 @@
 const redisVersion = require('redis/package.json').version
 const semver = require('semver')

+if (process.env.GITHUB_ACTIONS === 'true' && process.platform === 'win32') {
+  console.log('# SKIP: GH Actions do not support docker services on Windows')
+  process.exit(0)
+}
+
 if (semver.lt(redisVersion, '4.0.0')) {
   console.log('# SKIP: skipping redis.test.js tests <4.0.0')
   process.exit(0)

We'd need to do this to a lot of test files, but that should be okay. I can help with this if you like.
The easiest way to get the list of relevant test files to modify is by looking at what test files are run for each of these services/modules in ".tav.yml".

  • If so, is it needed to run always on PRs? Or nightly could be a good solution?

Does running it only nightly (and not on PRs) help with this issue? We still wouldn't be able to run the full test suite on a GitHub Actions windows worker.

@v1v
Copy link
Member Author

v1v commented Mar 29, 2023

Thanks @trentm , I think I can take it from here.

On the other hand, I'd like to extend the proposal to support external services on Windows if needed in the future with something like:

if (process.env.SKIP_EXTERNAL_SERVICE_TEST === 'true' && process.platform === 'win32') {

Then we could support the below workflows:

  1. Running on PRs for Windows with SKIP_EXTERNAL_SERVICE_TEST=true
  2. Running on main for windows.

The 1) will be done within this PR.

The 2) will require running on private runners or the Cloud but that's something we could implement in a follow up, or at least think about how to support it. One of the reasons for splitting that logic in two different workflows is the one accessing to secrets, that's something we try to be cautious and avoid running on PRs with access to any secrets.

What do you think?

@trentm
Copy link
Member

trentm commented Mar 29, 2023

On the other hand, I'd like to extend the proposal to support external services on Windows if needed in the future with something like:

Personally I would bias to not adding a SKIP_EXTERNAL_SERVICE_TEST abstraction unless we think it is likely that we will do 2) running on private runners or the Cloud. However, I am not opposed.

If you choose to add the abstraction, perhaps a name like TEST_NO_EXTERNAL_SERVICES.
Also, shouldn't that envvar encode whether this is for Windows? I.e. it would be:

if (process.env.TEST_NO_EXTERNAL_SERVICES === 'true') { ...

@v1v
Copy link
Member Author

v1v commented Mar 29, 2023

I agree, if there is no need for testing those external services on Windows, then we can keep it simple.

So I'll use your proposal and we can revisit in the future if there is need to support testing the external services on Windows.

@trentm
Copy link
Member

trentm commented Mar 29, 2023

if there is no need for testing those external services on Windows, then we can keep it simple.

It would be nice, but it isn't worth the effort right now. If we hear from any customers about issues using on Windows, then I'll bring it up again.

Thanks.

v1v added 3 commits March 30, 2023 09:40
* upstream/main:
  test: bump threshold to avoid spurious failure for possibly slow CI (#3225)
  test: workaround spurious test failures in http2 tests with node v8 (#3226)
those tests are the ones defined in the list of modules in .github/workflows/tav.yml
@v1v
Copy link
Member Author

v1v commented Mar 30, 2023

I've just excluded the tests on Windows for the below instrumentations:

@elastic
apollo-server-express
aws-sdk
bluebird
cassandra-driver
elasticsearch
express-graphql
express-queue
express
fastify
finalhandler
generic-pool
graphql
handlebars
ioredis
koa-router
mimic-response
mongodb-core
mongodb
mysql
mysql2
next
pg
pug
redis-2-3
redis
redis4-legacy
restifyframework
tedious
undici
ws

See 1b54c5f

@v1v
Copy link
Member Author

v1v commented Mar 30, 2023

There are some test failures on Windows:

The below one seems to be solved with c44ca29:

2023-03-30T08:20:25.7631353Z not ok 2 body should be uncompressed
2023-03-30T08:20:25.7631824Z   ---
2023-03-30T08:20:25.7632138Z     operator: equal
2023-03-30T08:20:25.7632507Z     expected: '/*\n * Copyri'
2023-03-30T08:20:25.7632874Z     actual:   '/*\r\n * Copyr'
2023-03-30T08:20:25.7633611Z     at: <anonymous> (D:\a\apm-agent-nodejs\apm-agent-nodejs\test\instrumentation\modules\http\github-423.test.js:57:11)
2023-03-30T08:20:25.7634081Z     stack: |-
2023-03-30T08:20:25.7634384Z       Error: body should be uncompressed
2023-03-30T08:20:25.7634970Z           at Test.assert [as _assert] (D:\a\apm-agent-nodejs\apm-agent-nodejs\node_modules\tape\lib\test.js:312:48)
2023-03-30T08:20:25.7635615Z           at Test.bound [as _assert] (D:\a\apm-agent-nodejs\apm-agent-nodejs\node_modules\tape\lib\test.js:95:17)
2023-03-30T08:20:25.7636238Z           at Test.strictEqual (D:\a\apm-agent-nodejs\apm-agent-nodejs\node_modules\tape\lib\test.js:476:7)
2023-03-30T08:20:25.7636882Z           at Test.bound [as strictEqual] (D:\a\apm-agent-nodejs\apm-agent-nodejs\node_modules\tape\lib\test.js:95:17)
2023-03-30T08:20:25.7637522Z           at D:\a\apm-agent-nodejs\apm-agent-nodejs\test\instrumentation\modules\http\github-423.test.js:57:11
2023-03-30T08:20:25.7638031Z           at processTicksAndRejections (internal/process/task_queues.js:95:5)
2023-03-30T08:20:25.7638354Z   ...

but there is now another one:

2023-03-30T08:47:15.6808149Z running test: cd . && node --unhandled-rejections=strict test\opentelemetry-bridge\fixtures.test.js
2023-03-30T08:47:15.9155107Z TAP version 13
...
2023-03-30T08:47:19.4083062Z D:\a\apm-agent-nodejs\apm-agent-nodejs\test\opentelemetry-bridge\fixtures.test.js:120
2023-03-30T08:47:19.4083778Z       const portB = tas[3].span.context.destination.port
2023-03-30T08:47:19.4084476Z                                 ^
2023-03-30T08:47:19.4084751Z 
2023-03-30T08:47:19.4085649Z TypeError: Cannot read property 'context' of undefined
2023-03-30T08:47:19.4086799Z     at Object.check (D:\a\apm-agent-nodejs\apm-agent-nodejs\test\opentelemetry-bridge\fixtures.test.js:120:33)
2023-03-30T08:47:19.4087779Z     at done (D:\a\apm-agent-nodejs\apm-agent-nodejs\test\opentelemetry-bridge\fixtures.test.js:427:15)
2023-03-30T08:47:19.4088457Z     at ChildProcess.exithandler (child_process.js:374:7)
2023-03-30T08:47:19.4089040Z     at ChildProcess.emit (events.js:400:28)
2023-03-30T08:47:19.4089549Z     at maybeClose (internal/child_process.js:1088:16)
2023-03-30T08:47:19.4090154Z     at Process.ChildProcess._handle.onexit (internal/child_process.js:296:5)
2023-03-30T08:47:19.4216014Z D:\a\apm-agent-nodejs\apm-agent-nodejs\test\test.js:218
2023-03-30T08:47:19.4216534Z     if (err) throw err
2023-03-30T08:47:19.4216823Z              ^
2023-03-30T08:47:19.4217200Z 
2023-03-30T08:47:19.4217788Z Error: non-zero error code
2023-03-30T08:47:19.4221637Z     at ChildProcess.<anonymous> (D:\a\apm-agent-nodejs\apm-agent-nodejs\test\test.js:106:21)
2023-03-30T08:47:19.4222783Z     at ChildProcess.emit (events.js:400:28)
2023-03-30T08:47:19.4223307Z     at maybeClose (internal/child_process.js:1088:16)
2023-03-30T08:47:19.4223920Z     at Process.ChildProcess._handle.onexit (internal/child_process.js:296:5) {
2023-03-30T08:47:19.4224487Z   code: 'ENONZERO',
2023-03-30T08:47:19.4225110Z   exitCode: 1
2023-03-30T08:47:19.4225496Z }

any hints?

.ci/Jenkinsfile Outdated Show resolved Hide resolved
@trentm
Copy link
Member

trentm commented Mar 30, 2023

@v1v Instead of the .gitattributes change in c44ca29 can we please make this change:

diff --git a/test/instrumentation/modules/http/github-423.test.js b/test/instrumentation/modules/http/github-423.test.js
index a00bebfb..e74e2fe5 100644
--- a/test/instrumentation/modules/http/github-423.test.js
+++ b/test/instrumentation/modules/http/github-423.test.js
@@ -54,7 +54,7 @@ test('https://github.com/elastic/apm-agent-nodejs/issues/423', function (t) {
     var server = http.createServer(function (req, res) {
       got(url).then(function (response) {
         t.strictEqual(response.body.length, fileSize, 'body should be expected size')
-        t.strictEqual(response.body.slice(0, 12), '/*\n * Copyri', 'body should be uncompressed')
+        t.ok(response.body.includes('Copyright Elasticsearch'), 'body should be uncompressed')
         res.end()
       })
     })

@trentm
Copy link
Member

trentm commented Mar 30, 2023

@v1v Instead of the .gitattributes change in c44ca29 can we please make this change:

I've done it.

I'll try to fix the "test\opentelemetry-bridge\fixtures.test.js" failure as well.

… apparently unreliable on the GH Actions Windows runners
.github/workflows/test.yml Outdated Show resolved Hide resolved
@v1v v1v self-assigned this Mar 31, 2023
@v1v v1v marked this pull request as ready for review March 31, 2023 06:30
@v1v v1v requested a review from a team March 31, 2023 06:30
Co-authored-by: Ivan Fernandez Calvo <[email protected]>
@v1v v1v merged commit dc7c03f into main Mar 31, 2023
@v1v v1v deleted the feature/run-on-windows branch March 31, 2023 09:54
fpm-peter pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants