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

Add node test for checking exported symbols #2835

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
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
4 changes: 4 additions & 0 deletions .github/workflows/node.yml
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ jobs:
github-token: ${{ secrets.GITHUB_TOKEN }}
engine-version: ${{ matrix.engine.version }}

- name: Run npm install in ./npm/glide package
run: npm install
working-directory: ./node/npm/glide

- name: test
run: npm test
working-directory: ./node
Expand Down
1 change: 0 additions & 1 deletion .github/workflows/npm-cd.yml
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,6 @@ jobs:
working-directory: ./node/npm/glide
run: |
export pkg_name=valkey-glide

echo "The workflow is: ${{env.EVENT_NAME}}"
if ${{ env.EVENT_NAME == 'workflow_dispatch' }}; then
R_VERSION="${{ env.INPUT_VERSION }}"
Expand Down
66 changes: 44 additions & 22 deletions node/DEVELOPER.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ Software Dependencies

If your Nodejs version is below the supported version specified in the client's [documentation](https://github.com/valkey-io/valkey-glide/blob/main/node/README.md#nodejs-supported-version), you can upgrade it using [NVM](https://github.com/nvm-sh/nvm?tab=readme-ov-file#install--update-script).

- npm
- git
- GCC
- pkg-config
- protoc (protobuf compiler)
- openssl
- openssl-dev
- rustup
- npm
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it done by a linter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

- git
- GCC
- pkg-config
- protoc (protobuf compiler)
- openssl
- openssl-dev
- rustup

**Dependencies installation for Ubuntu**

Expand Down Expand Up @@ -107,16 +107,38 @@ Before starting this step, make sure you've installed all software requirments.
5. Integrating the built GLIDE package into your project:
Add the package to your project using the folder path with the command `npm install <path to GLIDE>/node`.

- For a fast build, execute `npm run build`. This will perform a full, unoptimized build, which is suitable for developing tests. Keep in mind that performance is significantly affected in an unoptimized build, so it's required to build with the `build:release` or `build:benchmark` option when measuring performance.
- If your modifications are limited to the TypeScript code, run `npm run build-external` to build the external package without rebuilding the internal package.
- If your modifications are limited to the Rust code, execute `npm run build-internal` to build the internal package and generate TypeScript code.
- To generate Node's protobuf files, execute `npm run build-protobuf`. Keep in mind that protobuf files are generated as part of full builds (e.g., `build`, `build:release`, etc.).
6. Testing the GLIDE npm package locally:
The `node/npm/glide` folder contains a package wrapper that re-exports all the native bindings. To build and test this package, follow these steps:

```bash

Build node package:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move nodes outside of code block or add a comment

cd node
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove a black space ident in the entire block

Suggested change
cd node
cd node

export nativeStr=darwin-x64; export scope=@valkey/;
envsubst < package.json.tmpl > "package.json"
npm run build
npm run build:release
Comment on lines +119 to +120
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why need to build it twice?


In npm/glide package run the following commands:
cd ../npm/glide
export pkg_name=valkey-glide; export package_version=99.99.0; export scope=@valkey/;
envsubst < package.json.tmpl > "package.json"
npm run build
npm run build:test
npm run test
npm run test -- --testNamePattern="Exported symbols test"
Comment on lines +128 to +129
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this test run in scope of npm run test task?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I will add a comment, for running a specific test.

```

- For a fast build, execute `npm run build`. This will perform a full, unoptimized build, which is suitable for developing tests. Keep in mind that performance is significantly affected in an unoptimized build, so it's required to build with the `build:release` or `build:benchmark` option when measuring performance.
- If your modifications are limited to the TypeScript code, run `npm run build-external` to build the external package without rebuilding the internal package.
- If your modifications are limited to the Rust code, execute `npm run build-internal` to build the internal package and generate TypeScript code.
- To generate Node's protobuf files, execute `npm run build-protobuf`. Keep in mind that protobuf files are generated as part of full builds (e.g., `build`, `build:release`, etc.).

> Note: Once building completed, you'll find the compiled JavaScript code in the `node/build-ts` folder.

### Troubleshooting

- If the build fails after running `npx tsc` because `glide-rs` isn't found, check if your npm version is in the range 9.0.0-9.4.1, and if so, upgrade it. 9.4.2 contains a fix to a change introduced in 9.0.0 that is required in order to build the library.
- If the build fails after running `npx tsc` because `glide-rs` isn't found, check if your npm version is in the range 9.0.0-9.4.1, and if so, upgrade it. 9.4.2 contains a fix to a change introduced in 9.0.0 that is required in order to build the library.

### Test

Expand Down Expand Up @@ -202,13 +224,13 @@ Development on the Node wrapper may involve changes in either the TypeScript or

**TypeScript:**

- ESLint
- Prettier
- ESLint
- Prettier

**Rust:**

- clippy
- fmt
- clippy
- fmt

#### Running the linters

Expand All @@ -231,8 +253,8 @@ Development on the Node wrapper may involve changes in either the TypeScript or

### Recommended extensions for VS Code

- [Prettier - Code formatter](https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode) - JavaScript / TypeScript formatter.
- [ESLint](https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint) - linter.
- [Jest Runner](https://marketplace.visualstudio.com/items?itemName=firsttris.vscode-jest-runner) - in-editor test runner.
- [Jest Test Explorer](https://marketplace.visualstudio.com/items?itemName=kavod-io.vscode-jest-test-adapter) - adapter to the VSCode testing UI.
- [rust-analyzer](https://marketplace.visualstudio.com/items?itemName=rust-lang.rust-analyzer) - Rust language support for VSCode.
- [Prettier - Code formatter](https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode) - JavaScript / TypeScript formatter.
- [ESLint](https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint) - linter.
- [Jest Runner](https://marketplace.visualstudio.com/items?itemName=firsttris.vscode-jest-runner) - in-editor test runner.
- [Jest Test Explorer](https://marketplace.visualstudio.com/items?itemName=kavod-io.vscode-jest-test-adapter) - adapter to the VSCode testing UI.
- [rust-analyzer](https://marketplace.visualstudio.com/items?itemName=rust-lang.rust-analyzer) - Rust language support for VSCode.
8 changes: 4 additions & 4 deletions node/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,16 @@ The release of Valkey GLIDE was tested on the following platforms:

Linux:

- Ubuntu 22.04.1 (x86_64 and aarch64)
- Amazon Linux 2023 (AL2023) (x86_64)
- Ubuntu 22.04.1 (x86_64 and aarch64)
- Amazon Linux 2023 (AL2023) (x86_64)

macOS:

- macOS 14.7 (Apple silicon/aarch_64)
- macOS 14.7 (Apple silicon/aarch_64)

Alpine:

- node:alpine (default on aarch64 and x86_64)
- node:alpine (default on aarch64 and x86_64)

## NodeJS supported version

Expand Down
108 changes: 57 additions & 51 deletions node/npm/glide/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,64 +11,70 @@ let globalObject = global as unknown;

/* eslint-disable @typescript-eslint/no-require-imports */
function loadNativeBinding() {
let nativeBinding = null;
const scope = process.env.scope || "@scope";
Copy link
Collaborator

Choose a reason for hiding this comment

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

move this to line 75 where it is used


switch (platform) {
case "linux":
switch (arch) {
case "x64":
switch (familySync()) {
case GLIBC:
nativeBinding = require("@scope/valkey-glide-linux-x64");
break;
case MUSL:
nativeBinding = require("@scope/valkey-glide-linux-musl-x64");
break;
default:
nativeBinding = require("@scope/valkey-glide-linux-x64");
break;
}
let nativeBinding = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be removed if it is not used until line 76

let nativeStr = process.env.native_binding;

break;
case "arm64":
switch (familySync()) {
case GLIBC:
nativeBinding = require("@scope/valkey-glide-linux-arm64");
break;
case MUSL:
nativeBinding = require("@scope/valkey-glide-linux-musl-arm64");
break;
default:
nativeBinding = require("@scope/valkey-glide-linux-arm64");
break;
}
if (nativeStr == undefined) {
switch (platform) {
case "linux":
switch (arch) {
case "x64":
switch (familySync()) {
case MUSL:
nativeStr = "linux-musl-x64";
break;
case GLIBC:
default:
nativeStr = "linux-x64";
break;
}

break;
default:
throw new Error(
`Unsupported OS: ${platform}, architecture: ${arch}`,
);
}
break;
case "arm64":
switch (familySync()) {
case MUSL:
nativeStr = "linux-musl-arm64";
break;
case GLIBC:
default:
nativeStr = "linux-arm64";
break;
}

break;
case "darwin":
switch (arch) {
case "arm64":
nativeBinding = require("@scope/valkey-glide-darwin-arm64");
break;
default:
throw new Error(
`Unsupported OS: ${platform}, architecture: ${arch}`,
);
}
break;
default:
throw new Error(
`Unsupported OS: ${platform}, architecture: ${arch}`,
);
}

break;
default:
throw new Error(
`Unsupported OS: ${platform}, architecture: ${arch}`,
);
break;
case "darwin":
switch (arch) {
case "x64":
nativeStr = "darwin-x64";
break;
case "arm64":
nativeStr = "darwin-arm64";
break;
default:
throw new Error(
`Unsupported OS: ${platform}, architecture: ${arch}`,
);
}

break;
default:
throw new Error(
`Unsupported OS: ${platform}, architecture: ${arch}`,
);
}
}

nativeBinding = require(`${scope}valkey-glide-${nativeStr}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
nativeBinding = require(`${scope}valkey-glide-${nativeStr}`);
const nativeBinding = require(`${scope}valkey-glide-${nativeStr}`);


if (!nativeBinding) {
throw new Error(`Failed to load native binding`);
}
Expand Down
32 changes: 32 additions & 0 deletions node/npm/glide/jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/* eslint no-undef: off */
module.exports = {
preset: "ts-jest",
transform: {
"^.+\\.(t|j)s$": ["ts-jest", { isolatedModules: true }],
},
testEnvironment: "node",
testRegex: "/tests/.*\\.(test|spec)?\\.(ts|tsx)$",
moduleFileExtensions: [
"ts",
"tsx",
"js",
"jsx",
"json",
"node",
"cjs",
"mjs",
],
testTimeout: 600000,
reporters: [
"default",
[
"./node_modules/jest-html-reporter",
{
includeFailureMsg: true,
includeSuiteFailure: true,
executionTimeWarningThreshold: 60,
sort: "status",
},
],
],
};
75 changes: 75 additions & 0 deletions node/npm/glide/package.json.tmpl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't you update node/npm/glide/package.json instead? If no, we have to maintain these 2 files in sync.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

package.json.tmpl has some changes to run this in local.
Later, we will find a solution that works for CI. All the env variables will need to decided accordingly.
Cleaning up the package.json will be part of that task.

Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
{
"name": "${scope}${pkg_name}",
"types": "build-ts/index.d.ts",
"version": "${package_version}",
"description": "General Language Independent Driver for the Enterprise (GLIDE) for Valkey",
"main": "build-ts/index.js",
"module": "build-ts/index.js",
"type": "commonjs",
"scripts": {
"lint": "eslint .",
"lint:fix": "eslint . --fix",
"clean": "rm -rf build-ts/",
"copy-declaration-files": "cp ../../build-ts/*.d.ts build-ts/ && cp ../../build-ts/src/*.d.ts build-ts/src/ && cp ../../build-ts/src/server-modules/*.d.ts build-ts/src/server-modules/",
"build": "tsc && mkdir -p build-ts/src && mkdir -p build-ts/src/server-modules && npm run copy-declaration-files",
"build:test": "npm i && npm run build",
"test": "jest --verbose"
},
"files": [
"/build-ts"
],
"repository": {
"type": "git",
"url": "git+https://github.com/valkey-io/valkey-glide.git"
},
"keywords": [
"valkey",
"valkeyClient",
"client",
"valkey-glide"
],
"author": "Valkey GLIDE Maintainers",
"license": "Apache-2.0",
"bugs": {
"url": "https://github.com/valkey-io/valkey-glide/issues"
},
"homepage": "https://github.com/valkey-io/valkey-glide#readme",
"devDependencies": {
"@jest/globals": "^29.7.0",
"@types/jest": "^29.5.14",
"ts-jest": "^29.2.5",
"jest": "^29.7.0",
"jest-html-reporter": "^3.10.2",
"@types/node": "^18.11.18",
"@typescript-eslint/eslint-plugin": "^5.48.0",
"@typescript-eslint/parser": "^5.48.0",
"eslint": "^8.31.0",
"typescript": "^4.9.4",
"${scope}valkey-glide-${nativeStr}": "../.."
},
"optionalDependencies": {
"${scope}valkey-glide-darwin-arm64": "${package_version}",
"${scope}valkey-glide-darwin-x64": "${package_version}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"${scope}valkey-glide-darwin-x64": "${package_version}",

This one is not supported now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove. Thanks.

"${scope}valkey-glide-linux-arm64": "${package_version}",
"${scope}valkey-glide-linux-x64": "${package_version}",
"${scope}valkey-glide-linux-musl-arm64": "${package_version}",
"${scope}valkey-glide-linux-musl-x64": "${package_version}"
},
"eslintConfig": {
"extends": [
"eslint:recommended",
"plugin:@typescript-eslint/recommended"
],
"parser": "@typescript-eslint/parser",
"plugins": [
"@typescript-eslint"
],
"ignorePatterns": [
"build-ts/*"
],
"root": true
},
"dependencies": {
"detect-libc": "^2.0.3"
}
}
Loading
Loading