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 debugger extension tests #2292

Merged

Conversation

MrSarius
Copy link
Contributor

@MrSarius MrSarius commented Jun 13, 2023

This PR adds tests for #2219. The VS Code extension has not had any tests yet. The tests only cover code that was touched by me.

I have added the tests to the compilation_on_android_ubuntu.yml workflow. The first run will take about two hours, since LLDB is built from scratch. Later, the build should be cached and the whole job should not take more than three minutes.

Core of the PR is an integration test that boots up vs code and lets it debug a test WASM file.

Run Tests locally

Download the LLDB build for your platform from my pipeline.

cd ${WAMR_ROOT}/test-tools/wamr-ide/VSCode-Extension/resource/debug
wget ${LLDB_FOR_YOUR_PLATFORM}.tar.gz -O lldb.tar.gz
tar -xvf lldb.tar.gz
cp -R wamr-lldb/* ./${YOUR_PLATFORM}

Make sure wasm32-wasi target is available.

rustup target add wasm32-wasi

Build iwasm with source debugging feature

cd ${WAMR_ROOT}/product-mini/platforms/${YOUR_PLATFORM}
mkdir build && cd build
cmake .. -DWAMR_BUILD_DEBUG_INTERP=1
make

Note: On MacOS M1 environment, pass the additional -DWAMR_DISABLE_HW_BOUND_CHECK=1 cmake configuration.

Install npm dependencies and run tests.

cd ${WAMR_ROOT}/test-tools/wamr-ide/VSCode-Extension
npm install
npm run test

@MrSarius MrSarius force-pushed the benjuri/add-tests-vscode-extension branch 18 times, most recently from 527b16d to a7cf180 Compare June 14, 2023 20:51
@MrSarius MrSarius changed the title Add code extension tests Add vs code extension tests Jun 14, 2023
@MrSarius MrSarius changed the title Add vs code extension tests Add debugger extension tests Jun 14, 2023
@MrSarius MrSarius force-pushed the benjuri/add-tests-vscode-extension branch 4 times, most recently from c54891f to f5741d0 Compare June 15, 2023 12:48
@MrSarius MrSarius force-pushed the benjuri/add-tests-vscode-extension branch from f5741d0 to 39b31d4 Compare June 15, 2023 13:10
@@ -565,3 +568,117 @@ jobs:
if: env.TEST_ON_X86_32 == 'true'
run: ./test_wamr.sh ${{ env.X86_32_TARGET_TEST_OPTIONS }} ${{ matrix.test_option }} -t ${{ matrix.running_mode }}
working-directory: ./tests/wamr-test-suites

test-wamr-ide:
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add it to nightly_run instead of per-PR if it's 2hrs?

Copy link
Contributor

Choose a reason for hiding this comment

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

LLDB version doesn't get updated that often, if we cache it (the first time a PR for the update is created), it should be fine

Copy link
Contributor Author

@MrSarius MrSarius Jun 16, 2023

Choose a reason for hiding this comment

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

I also think it should be fine.

The LLDB patch file was changed four times in the last 2 years.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you got it

@MrSarius MrSarius marked this pull request as ready for review June 16, 2023 09:29
@TianlongLiang
Copy link
Contributor

To run tests locally, the node version has to be 16.x+(GitHub Actions use 18.16.0), is there a way to add it in package.json to ensure the user's node meets the requirement?

What I can think of is to add the engines field in your package.json file like:

    "engines": {
        "vscode": "^1.59.0",
        "node": "^16.0.0"
    },

and add engine-strict option in the new .npmrc file:

engine-strict=true

This will force npm to err for node versions lower than 16.x. I think it's better to err by setting engines rather than warn by setting devDependencies

What do you think? Is there any other way to achieve this?

@MrSarius MrSarius force-pushed the benjuri/add-tests-vscode-extension branch from f7d943a to 4e1469e Compare June 19, 2023 14:38
@MrSarius
Copy link
Contributor Author

MrSarius commented Jun 19, 2023

Hi @TianlongLiang,
Thanks for the feedback. All Done!

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh wenyongh merged commit 72fc872 into bytecodealliance:main Jun 20, 2023
@Zzzabiyaka
Copy link
Contributor

does it worth adding the same tests for macOS CI job if it's supported? @MrSarius

@MrSarius
Copy link
Contributor Author

MrSarius commented Jun 20, 2023

In general, of course, the integration tests could also find major problems in the LLDB build (crashes etc.). That could be helpful for all platforms that are suppported.

But in the context of the extension, that doesn't add much value compared to the extra effort it means (building and caching LLDB for Macos)

wenyongh pushed a commit that referenced this pull request Jun 21, 2023
Since building the extension now depends on node >=16 (see #2292),
build_wamr_vscode_ext.yml will fail on the next release without this change.
@TianlongLiang
Copy link
Contributor

Hi, @MrSarius, after the recent update of VS Code, for version above 1.86.0, it seems that the format conversion in formatters/rust.py failed, therefore it returns the raw format and assertion fails during the value check process.

      The Deque summary string looks different than expected
      + expected - actual

      -alloc::collections::vec_deque::VecDeque<int, alloc::alloc::Global> @ 0xfff1c
      + (5) VecDeque[1, 2, 3, 4, 5]

For fixing the CI error, could you please let me know your opinion? Do you think this is mainly a VS Code bug? If so, maybe we can safely ignore it for a while and use the fixed VS Code version. Or is it a major interface change that needs modification on our side?

victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
This PR adds tests for bytecodealliance#2219 by changing the `compilation_on_android_ubuntu.yml` workflow.
The first run will take about two hours, since LLDB is built from scratch. Later, the build is
cached and the whole job should not take more than three minutes.

Core of the PR is an integration test that boots up vscode and lets it debug a test WASM file.
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Since building the extension now depends on node >=16 (see bytecodealliance#2292),
build_wamr_vscode_ext.yml will fail on the next release without this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants