Skip to content

Conversation

@mcbarton
Copy link
Collaborator

Description

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

Once this PR in xeus-cpp compiler-research/xeus-cpp#277 is ready and merged, doctest will need to be added Emscripten conda environment in 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

@codecov
Copy link

codecov bot commented Mar 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.71%. Comparing base (993550c) to head (4fad8f1).
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #529   +/-   ##
=======================================
  Coverage   72.71%   72.71%           
=======================================
  Files           9        9           
  Lines        3618     3618           
=======================================
  Hits         2631     2631           
  Misses        987      987           
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vgvassilev
Copy link
Contributor

CppInterOp does not depend on doctest. Why do we need this?

@mcbarton
Copy link
Collaborator Author

mcbarton commented Mar 17, 2025

CppInterOp does not depend on doctest. Why do we need this?

@vgvassilev Because xeus-cpp does, and once the automated Emscripten tests PR reference in the PR description is in, xeus-cpp will try and find doctest to link against for the tests (tests are on and build by default in the PR). We could in theory set the xeus-cpp tests to OFF by a cmake option, but by keeping them on we will be able to have another check that no changes to CppInterOp break the Emscripten build of xeus-cpp.

@vgvassilev
Copy link
Contributor

I am not sure I completely understand that dependency. The CppInterOp tests and the xeus-cpp tests should be separate and independent from each other. If there is some required dependency then we have a problem.

@mcbarton
Copy link
Collaborator Author

mcbarton commented Mar 17, 2025

I am not sure I completely understand that dependency. The CppInterOp tests and the xeus-cpp tests should be separate and independent from each other. If there is some required dependency then we have a problem.

I can change this PR to turn off xeus-cpp tests regardless of what the default value of XEUS_CPP_BUILD_TESTS in xeus-cpp is, but something will need to be done in preparation for the PR mentioned in this PR description. If no change is made, the Emscripten build, and deploy workflow will break once its in.

One thing to note is that the environment-wasm.yml file is only in this repo because of CppInterOps xeus-cpp-lite build. Nothing in that file is a dependency of CppInterOp.

@vgvassilev
Copy link
Contributor

Now I think I understand. That’s a ci dependency. Okay I think the pr is good to go from my side.

@mcbarton
Copy link
Collaborator Author

Now I think I understand. That’s a ci dependency. Okay I think the pr is good to go from my side.

Would you like to wait for the automated wasm tests PR in xeus-cpp to go in first, or can this go on now?

@vgvassilev
Copy link
Contributor

This can go.

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.

Lgtm.

@mcbarton mcbarton merged commit f0f59f2 into compiler-research:main Mar 18, 2025
71 checks passed
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