Enable the %%file xmagic

[faze-geek]

Description

Towards #176

Type of change

Please tick all options which are relevant.

• Bug fix
• New feature
• Added/removed dependencies
• Required documentation updates

[faze-geek]

Let me know if I need to add more tests. I wrote them for whatever functionalities were supported in the documentation.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.80%. Comparing base (859a1b9) to head (41b13d8).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
+ Coverage   80.78%   80.80%   +0.01%     
==========================================
  Files          19       19              
  Lines         973      974       +1     
  Branches       93       93              
==========================================
+ Hits          786      787       +1     
  Misses        187      187              

Files with missing lines	Coverage Δ
src/xinterpreter.cpp	91.39% <100.00%> (+0.04%)	⬆️

Files with missing lines	Coverage Δ
src/xinterpreter.cpp	91.39% <100.00%> (+0.04%)	⬆️

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[github-actions]

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

[anutosh491]

Hmmm, I know that EMSCRIPTEN supports file utilities (through both libc and libcxx) making use of the virtual file system it provides. So technically we should be able to compile and run this using std::ofstream

But untill xeus-cpp-lite takes good shape (and we don't have a means to test the above out) I would keep this out of emscripten.

[anutosh491]

Please make the following changes

1. In cmakelists.txt : Move oss header & source to the not emscripten case.

set(XEUS_CPP_HEADERS
    ....
    #src/xmagics/os.hpp
)

set(XEUS_CPP_SRC
    ....
    src/xinterpreter.cpp
    src/xmagics/os.cpp
    ....
)

if(NOT EMSCRIPTEN)
    list(APPEND XEUS_CPP_SRC
        src/xmagics/xassist.hpp
        src/xmagics/xassist.cpp
    )
endif()

2. in src/xinterpreter.cpp : Do the same as above.

#include "xmagics/os.hpp"
#ifndef EMSCRIPTEN
#include "xmagics/xassist.hpp"
#endif

[anutosh491]

Apart from that. The tests look decent to me. Maybe @mcbarton could also review this for us !

[vgvassilev]

@mcbarton, do we not have code coverage bots checking the test suite coverage for xeus-cpp? I remember we had that but maybe I am confusing myself...

[mcbarton]

@mcbarton, do we not have code coverage bots checking the test suite coverage for xeus-cpp? I remember we had that but maybe I am confusing myself...

@vgvassilev Isn't the test coverage being shown in this message? #181 (comment)

[vgvassilev]

@mcbarton, do we not have code coverage bots checking the test suite coverage for xeus-cpp? I remember we had that but maybe I am confusing myself...

@vgvassilev Isn't the test coverage being shown in this message? #181 (comment)

Sorry, I am blind... In clad we had these listed as part of the bot status...

[github-actions]

clang-tidy made some suggestions

[github-actions]

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

[anutosh491]

The tests look decent to me. Approving !

[github-actions]

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

[github-actions]

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

[anutosh491]

I am guessing this is ready ?

[github-actions]

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

[vgvassilev]

The suggested by clang-format changes look good.

[faze-geek]

The suggested by clang-format changes look good.

Done !

[github-actions]

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

[anutosh491]

I see a duplication of tests here. We already had tests for the writefile operation.

xeus-cpp/test/test_interpreter.cpp

Lines 527 to 590 in ca8c42e

	TEST_SUITE("os")
	{
	TEST_CASE("write_new_file") {
	xcpp::writefile wf;
	std::string line = "filename testfile.txt -h";
	std::string cell = "Hello, World!";
	wf(line, cell);
	std::ifstream infile("testfile.txt");
	REQUIRE(infile.good() == true);
	infile.close();
	std::remove("testfile.txt");
	}
	TEST_CASE("overwrite_file") {
	xcpp::writefile wf;
	std::string line = "filename testfile.txt";
	std::string cell = "Hello, World!";
	wf(line, cell);
	std::string overwrite_cell = "Overwrite test";
	wf(line, overwrite_cell);
	std::ifstream infile("testfile.txt");
	std::string content;
	std::getline(infile, content);
	REQUIRE(content == overwrite_cell);
	infile.close();
	std::remove("testfile.txt");
	}
	TEST_CASE("append_file") {
	xcpp::writefile wf;
	std::string line = "filename testfile.txt";
	std::string cell = "Hello, World!";
	wf(line, cell);
	std::string append_line = "filename testfile.txt --append";
	std::string append_cell = "Hello, again!";
	wf(append_line, append_cell);
	std::ifstream infile("testfile.txt");
	std::vector<std::string> lines;
	std::string content;
	while(std::getline(infile, content)) {
	lines.push_back(content);
	}
	REQUIRE(lines[0] == cell);
	REQUIRE(lines[1] == append_cell);
	infile.close();
	std::remove("testfile.txt");
	}
	}

Can someone confirm this for me ? The tests look similar though tests added in this PR looks better framed. Maybe @kr-2003 would like to help (get rid of the first set of tests)

[kr-2003]

I see a duplication of tests here. We already had tests for the writefile operation.

xeus-cpp/test/test_interpreter.cpp

Lines 527 to 590 in ca8c42e

	TEST_SUITE("os")
	{
	TEST_CASE("write_new_file") {
	xcpp::writefile wf;
	std::string line = "filename testfile.txt -h";
	std::string cell = "Hello, World!";
	wf(line, cell);
	std::ifstream infile("testfile.txt");
	REQUIRE(infile.good() == true);
	infile.close();
	std::remove("testfile.txt");
	}
	TEST_CASE("overwrite_file") {
	xcpp::writefile wf;
	std::string line = "filename testfile.txt";
	std::string cell = "Hello, World!";
	wf(line, cell);
	std::string overwrite_cell = "Overwrite test";
	wf(line, overwrite_cell);
	std::ifstream infile("testfile.txt");
	std::string content;
	std::getline(infile, content);
	REQUIRE(content == overwrite_cell);
	infile.close();
	std::remove("testfile.txt");
	}
	TEST_CASE("append_file") {
	xcpp::writefile wf;
	std::string line = "filename testfile.txt";
	std::string cell = "Hello, World!";
	wf(line, cell);
	std::string append_line = "filename testfile.txt --append";
	std::string append_cell = "Hello, again!";
	wf(append_line, append_cell);
	std::ifstream infile("testfile.txt");
	std::vector<std::string> lines;
	std::string content;
	while(std::getline(infile, content)) {
	lines.push_back(content);
	}
	REQUIRE(lines[0] == cell);
	REQUIRE(lines[1] == append_cell);
	infile.close();
	std::remove("testfile.txt");
	}
	}

Can someone confirm this for me ? The tests look similar though tests added in this PR looks better framed. Maybe @kr-2003 would like to help (get rid of the first set of tests)

Yeah. It seems like

xeus-cpp/test/test_interpreter.cpp

Line 985 in ca8c42e

TEST_SUITE("file") {

does the same tests. The only difference I can see is "removing the txt file after every subtest".

[anutosh491]

The only difference I can see is "removing the txt file after every subtest".

Not worth it. Let's just keep the better framed test