Fix Evaluate tests for emscripten build

[anutosh491]

Description

I realized technically this should work. The only reason it might not is if we fail to fetch the -std=c++14 arg which we should !

Creating the interpreter correctly should fix that I suppose

CppInterOp/lib/Interpreter/CppInterOp.cpp

Lines 2917 to 2923 in e1ace51

	TInterp_t CreateInterpreter(const std::vector<const char*>& Args /*={}*/,
	const std::vector<const char*>& GpuArgs /*={}*/) {
	std::string MainExecutableName =
	sys::fs::getMainExecutable(nullptr, nullptr);
	std::string ResourceDir = MakeResourcesPath();
	std::vector<const char *> ClingArgv = {"-resource-dir", ResourceDir.c_str(),
	"-std=c++14"};

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

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.93%. Comparing base (96045bd) to head (37fa886).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #544   +/-   ##
=======================================
  Coverage   75.93%   75.93%           
=======================================
  Files           9        9           
  Lines        3644     3644           
=======================================
  Hits         2767     2767           
  Misses        877      877           

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[anutosh491]

Strange, I won't expect the following to fail. Looks simple enough but it fails here

1: /home/runner/work/CppInterOp/CppInterOp/unittests/CppInterOp/InterpreterTest.cpp:80: Failure
1: Expected equality of these values:
1:   Cpp::Evaluate("int i = 11; ++i", &HadError)
1:     Which is: 2
1:   12
1: 
1: [  FAILED  ] InterpreterTest.Evaluate (262 ms)

Tried it locally too

Not sure why we get 2 in the first place !

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[anutosh491]

Okay this might be the weirdest thing I've come across in a while but I would like to bring your attention towards something (cc @vgvassilev @mcbarton )

If you go through the above comment of mine (#544 (comment)) you would realize that it is strange for something like this to fail. Its a simple Evaluate check which we have been doing since quite some time now and I also pasted an image of executing the same thing in xeus-cpp-lite.

Let me explain through a more clearer example on what is happening (cause the error here isn't justifying much)

Conside these 2 tests in FunctionReflectionTest

TEST(FunctionReflectionTest, Construct) {
..........

  Cpp::CreateInterpreter();

  Interp->declare(R"(
    #include <new>
    extern "C" int printf(const char*,...);
    class C {
      int x;
      C() {
        x = 12345;
        printf("Constructor Executed");
      }
    };
    )");

  testing::internal::CaptureStdout();
  Cpp::TCppScope_t scope = Cpp::GetNamed("C");
  Cpp::TCppObject_t object = Cpp::Construct(scope);
  EXPECT_TRUE(object != nullptr);
  std::string output = testing::internal::GetCapturedStdout();
  EXPECT_EQ(output, "Constructor Executed");
  output.clear();
  

TEST(FunctionReflectionTest, Destruct) {
.......

  Cpp::CreateInterpreter();

  Interp->declare(R"(
    #include <new>
    extern "C" int printf(const char*,...);
    class C {
      C() {}
      ~C() {
        printf("Destructor Executed");
      }
    };
    )");

  testing::internal::CaptureStdout();
  Cpp::TCppScope_t scope = Cpp::GetNamed("C");
  Cpp::TCppObject_t object = Cpp::Construct(scope);
  Cpp::Destruct(object, scope);
  std::string output = testing::internal::GetCapturedStdout();

  EXPECT_EQ(output, "Destructor Executed");

Now our emscripten build is good enough to address both these tests. But there is something going wrong with CaptureStdout and/or GetCapturedStdout provided by the emscripten build of gtest. So if Construct works, destruct won't work and would fail with this

1: /Users/anutosh491/work/CppInterOp/unittests/CppInterOp/FunctionReflectionTest.cpp:1593: Failure
1: Expected equality of these values:
1:   output
1:     Which is: "Constructor ExecutedDestructor Executed"
1:   "Destructor Executed"
1: 

So technically both these tests have the capacity to function perfectly on their own (and whichever would be placed first would work for now)

[anutosh491]

So basically what I realize is that its one of the GetCapturedStdout from EnumReflectionTest.cpp or FunctionReflectionTest.cpp that is spoiling this case with Evaluate. In simpler terms a side module loaded previously is spoiling the case here.

As a workaround (cause I am bullish on something like Evaluate to be test, I mean it is one of the basic things we should expect to work) I am just changing this for now

--- a/unittests/CppInterOp/CMakeLists.txt
+++ b/unittests/CppInterOp/CMakeLists.txt
@@ -10,9 +10,9 @@ else()
 endif()
 
 add_cppinterop_unittest(CppInterOpTests
-  EnumReflectionTest.cpp
-  FunctionReflectionTest.cpp
   InterpreterTest.cpp
+  FunctionReflectionTest.cpp
+  EnumReflectionTest.cpp

This ensures no flaky side module from the above affects stuff in InterpreterTest.cpp ( which technically is 3rd in place currently)

I know this might not be the "best" fix or so but i wouldn't have the time to look into what goes wrong in the emscripten build for gtest in the coming week or two. On the same note I think its really important to test something like Evaluate

So yeah changing the order changes nothing . Just gives us 1 more working test. Hope that's fine :)

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[vgvassilev]

We need to order this alphabetically.

[vgvassilev]

So basically what I realize is that its one of the GetCapturedStdout from EnumReflectionTest.cpp or FunctionReflectionTest.cpp that is spoiling this case with Evaluate. In simpler terms a side module loaded previously is spoiling the case here.

As a workaround (cause I am bullish on something like Evaluate to be test, I mean it is one of the basic things we should expect to work) I am just changing this for now

--- a/unittests/CppInterOp/CMakeLists.txt
+++ b/unittests/CppInterOp/CMakeLists.txt
@@ -10,9 +10,9 @@ else()
 endif()
 
 add_cppinterop_unittest(CppInterOpTests
-  EnumReflectionTest.cpp
-  FunctionReflectionTest.cpp
   InterpreterTest.cpp
+  FunctionReflectionTest.cpp
+  EnumReflectionTest.cpp

This ensures no flaky side module from the above affects stuff in InterpreterTest.cpp ( which technically is 3rd in place currently)

I know this might not be the "best" fix or so but i wouldn't have the time to look into what goes wrong in the emscripten build for gtest in the coming week or two. On the same note I think its really important to test something like Evaluate

So yeah changing the order changes nothing . Just gives us 1 more working test. Hope that's fine :)

That means that something is wrong with static initialization of the binary and it is better to get to the bottom of it.

[anutosh491]

That means that something is wrong with static initialization of the binary and it is better to get to the bottom of it.

Hmm, I am not sure that's the case. Check this #544 (comment)

[vgvassilev]

That means that something is wrong with static initialization of the binary and it is better to get to the bottom of it.

Hmm, I am not sure that's the case. Check this #544 (comment)

I did.

[anutosh491]

I did.

So basically we sure can run these tests. Looking as to how we can get round this !

[mcbarton]

Now our emscripten build is good enough to address both these tests. But there is something going wrong with CaptureStdout and/or GetCapturedStdout provided by the emscripten build of gtest.

@anutosh491 Since this PR went in, we updated the version of GoogleTests we are using here #552 . Have you checked that this bug hasn't been fixed in the latest version?

[anutosh491]

Hi,

As I wrote above I would fall short on time to investigate anything related to the emscripten build for gtest :(

As you introduced these set of tests would you like to take charge of fixing it ?

I think the "Evaluate" function is something that should absolutely be tested and not be skipped! (But yeah as Vassil pointed out my workaround might just be a cheap trick to get this done)

[vgvassilev]

We can move forward with the current approach only if we add a fixme in cmake and open a bug report explaining what we know and what needs to be investigated.