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 emscripten automated tests #483

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mcbarton
Copy link
Collaborator

Description

Please include a summary of changes, motivation and context for this PR.

My first PR to add automated emscripten tests became broken for unknown reasons (see #448). This PR is another attempt to added emscripten tests to CppInterOp

Fixes # (issue)

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Requires documentation updates

Testing

Please describe the test(s) that you added and ran to verify your changes.

Checklist

  • I have read the contribution guide recently

Copy link

codecov bot commented Jan 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.17%. Comparing base (b797dbb) to head (471ffd8).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #483   +/-   ##
=======================================
  Coverage   71.17%   71.17%           
=======================================
  Files           9        9           
  Lines        3552     3552           
=======================================
  Hits         2528     2528           
  Misses       1024     1024           

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

unittests/CppInterOp/InterpreterTest.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

unittests/CppInterOp/InterpreterTest.cpp Outdated Show resolved Hide resolved
@mcbarton mcbarton force-pushed the wip]-add-automated-wasm-tests branch 2 times, most recently from f3d8b34 to e211109 Compare January 26, 2025 12:50
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

unittests/CppInterOp/Utils.h Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -0,0 +1,6 @@
#include <gtest/gtest.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'gtest/gtest.h' file not found [clang-diagnostic-error]

#include <gtest/gtest.h>
         ^

@mcbarton mcbarton force-pushed the wip]-add-automated-wasm-tests branch 5 times, most recently from 027e9a3 to ab5976a Compare February 9, 2025 16:14
@mcbarton mcbarton changed the title [wip] Add emscripten automated tests Add emscripten automated tests Feb 9, 2025
@mcbarton mcbarton marked this pull request as ready for review February 9, 2025 16:15
@mcbarton mcbarton force-pushed the wip]-add-automated-wasm-tests branch from bba850e to 123e15c Compare February 9, 2025 16:27
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

I am adding a few comments from my partial review.

CMakeLists.txt Outdated Show resolved Hide resolved
cmake/modules/GoogleTest.cmake Outdated Show resolved Hide resolved
cmake/modules/GoogleTest.cmake Show resolved Hide resolved

if(EMSCRIPTEN)
# Read the undefined symbols from the text file
file(READ "${CMAKE_SOURCE_DIR}/lib/Interpreter/undefined_symbols.txt" SYMBOLS_LIST)
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

  • Doesn't the emscripten linker allow linker scripts?
  • This file should be called exports.ld or something like that...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not aware of linker scripts. Can you point me a resource about them? I'll remame the file as suggested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed point 2 of renaming the file. Point 1 will take some more investigation on my part. My initial attempts to use a linker script ended in failure. Apart from this point I believe this PR is ready for another review when your available.

unittests/CppInterOp/CMakeLists.txt Outdated Show resolved Hide resolved
unittests/CppInterOp/main.cpp Show resolved Hide resolved
@mcbarton mcbarton force-pushed the wip]-add-automated-wasm-tests branch from 8ec905a to 827b696 Compare February 9, 2025 20:10
@mcbarton mcbarton requested a review from vgvassilev February 9, 2025 20:30
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.

2 participants