Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions .github/scripts/run_ft_tests.bash
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ if [ -z "$SETUP_CMD" ]; then
SETUP_CMD="start"
fi

UTAPI_INTERVAL_TEST_MODE=$1 npm $SETUP_CMD 2>&1 | tee -a "setup_$2.log" &
# Redirect stderr to a separate file (without "Killed" to look for warnings / error messages)
# While still keeping it in terminal
# ignore punycode warning from [email protected] in utapiV2 tests

UTAPI_INTERVAL_TEST_MODE=$1 npm $SETUP_CMD \
2> >(grep -v -E "^Killed$|--trace-deprecation|punycode" | tee -a "setup_$2.stderr.log" >&2) \
| tee -a "setup_$2.log" &

bash tests/utils/wait_for_local_port.bash $PORT 40
UTAPI_INTERVAL_TEST_MODE=$1 npm run $2 | tee -a "test_$2.log"

UTAPI_INTERVAL_TEST_MODE=$1 npm run $2 \
2> >(tee -a "test_$2.stderr.log" >&2) \
| tee -a "test_$2.log"
22 changes: 22 additions & 0 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ on:
required: false
description: Timeout for ssh connection to worker (minutes)
default: 30

env:
AWS_SDK_JS_SUPPRESS_MAINTENANCE_MODE_MESSAGE: '1'
# NODE_OPTIONS: '--trace-warnings'

jobs:
build-ci:
uses: ./.github/workflows/build-ci.yaml
Expand Down Expand Up @@ -137,6 +142,11 @@ jobs:
- name: ${{ matrix.test.name }}
run: ${{ matrix.test.command }}
env: ${{ matrix.test.env }}
- name: Print stderr logs
run: grep -H "" *.stderr.log || true
if: always()
- name: Error if stderr logs are not empty
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a bit extreme to fail the test? IDK, just a personal feeling, it's useful to have feedback on any new stderr message, maybe we can add a continue-on-error: true so it's visible but doesn't outright fail the test if the overall test suite is successful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the idea to catch immediately a warning, so we are not surprised later.

Then if we don't plan on fixing the warning, we can filter it out from the logs (38badb7)

A continue-on-error: true could be added at any moment if there are warnings that can't be fixed to pass the tests, but ideally I'd like to fail to make sure we notice it.

And I'd like to have this in other components as well in long term

run: if [ -n "$(find . -name '*.stderr.log' -size +0c)" ]; then exit 1; fi

tests-v2-with-vault:
needs:
Expand Down Expand Up @@ -242,6 +252,13 @@ jobs:
tmate-server-rsa-fingerprint: ${{ secrets.TMATE_SERVER_RSA_FINGERPRINT }}
tmate-server-ed25519-fingerprint: ${{ secrets.TMATE_SERVER_ED25519_FINGERPRINT }}
if: ${{ ( github.event.inputs.debug == true || github.event.inputs.debug == 'true' ) }}
- name: Print stderr logs
run: grep -H "" *.stderr.log || true
if: always()
- name: Error if stderr logs are not empty
run: if [ -n "$(find . -name '*.stderr.log' -size +0c)" ]; then exit 1; fi


tests-v2-without-sensision:
needs:
- build-ci
Expand Down Expand Up @@ -358,3 +375,8 @@ jobs:
tmate-server-rsa-fingerprint: ${{ secrets.TMATE_SERVER_RSA_FINGERPRINT }}
tmate-server-ed25519-fingerprint: ${{ secrets.TMATE_SERVER_ED25519_FINGERPRINT }}
if: ${{ ( github.event.inputs.debug == true || github.event.inputs.debug == 'true' ) }}
- name: Print stderr logs
run: grep -H "" *.stderr.log || true
if: always()
- name: Error if stderr logs are not empty
run: if [ -n "$(find . -name '*.stderr.log' -size +0c)" ]; then exit 1; fi
1 change: 1 addition & 0 deletions bin/ensureServiceUser
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// - deduplicate with Vault's seed script at https://github.com/scality/Vault/pull/1627
// - add permission boundaries to user when https://scality.atlassian.net/browse/VAULT-4 is implemented

process.env.AWS_SDK_JS_SUPPRESS_MAINTENANCE_MODE_MESSAGE = '1';
const { errors } = require('arsenal');
const { program } = require('commander');
const werelogs = require('werelogs');
Expand Down
3 changes: 3 additions & 0 deletions libV2/process.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ class Process extends EventEmitter {
async join() {
this.emit('exit');
await this._join();
['SIGINT', 'SIGQUIT', 'SIGTERM', 'uncaughtException'].forEach(eventName => {
process.removeAllListeners(eventName);
});
}

async _setup() {}
Expand Down
7 changes: 5 additions & 2 deletions libV2/redis.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
const EventEmitter = require('events');
const { callbackify, promisify } = require('util');
const { callbackify } = require('util');
const IORedis = require('ioredis');
const { jsutil } = require('arsenal');
const BackOff = require('backo');
const { whilst } = require('async');

const errors = require('./errors');
const { LoggerContext, asyncOrCallback } = require('./utils');
const flexiblePromisify = require('./utils/flexiblePromisify');

const moduleLogger = new LoggerContext({
module: 'redis',
Expand Down Expand Up @@ -201,7 +202,9 @@ class RedisClient extends EventEmitter {
if (callback !== undefined) {
// If a callback is provided `func` is assumed to also take a callback
// and is converted to a promise using promisify
return callbackify(this._call.bind(this))(promisify(func), callback);
// Note (DEP0174): flexiblePromisify avoids promisifying a function that already returns a promise
// With redisClientV2 func returns a promise even if there is a callback
return callbackify(this._call.bind(this))(flexiblePromisify(func), callback);
}
return this._call(func);
}
Expand Down
42 changes: 42 additions & 0 deletions libV2/utils/flexiblePromisify.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* Promisifies a function. If the function already returns a promise,
* it returns it as is. Handles both callbacks and promises.
*
* @param {Function} originalFn The function to promisify.
* @returns {Function} A function that returns a promise.
*/
module.exports = function flexiblePromisify(originalFn) {
// Check if the function provides a custom promise-based version.
// This is the same mechanism Node's util.promisify uses.
const customPromisified = originalFn[Symbol.for('nodejs.util.promisify.custom')];
if (typeof customPromisified === 'function') {
return customPromisified;
}

// Return the new promise-based wrapper function.
return function flexiblePromisifiedWrapper(...args) {
const thisCtx = this;

return new Promise((resolve, reject) => {
// 1. Callback will be used if `originalFn` is a callback-style function.
function callback(err, result) {
if (err) {
return reject(err);
}
return resolve(result);
}

// 2. Call the originalFn, with user's args AND our custom callback.
const potentialPromise = originalFn.apply(thisCtx, [...args, callback]);

// 3. If originalFn returned a promise, we use its result and ignore the callback.
if (potentialPromise && typeof potentialPromise.then === 'function') {
// The function returned a promise. We'll trust it as the source of truth.
potentialPromise.then(resolve, reject);
}

// If the function did NOT return a promise (i.e., it's a standard callback function),
// then our promise is already wired up to be resolved or rejected by the `callback` we passed in.
});
};
};
Loading
Loading