Skip to content

Conversation

brauliorivas
Copy link
Contributor

This pull request adds test cases for a few functions in the xcpp library.

should_print_version Test Suite: Tests verify if the function correctly identifies the presence or absence of the --version argument.

build_interpreter Test Suite: A test case checks if the function returns a non-null pointer when valid arguments are passed.

clone_magics_manager Test Suite: Tests ensure the function correctly returns a non-null pointer and that the cloned object matches the type of the original magics_manager.

is_match Test Suite: Verifies the function correctly identifies strings that match the regex pattern used in xmagics_manager.

register_and_access Test Suite: This test verifies the correct registration and access of a xpreamble object within the xpreamble_manager. It ensures that a xpreamble object can be registered with a name and subsequently accessed using the same name.

xbuffer Test Suite: The new test cases cover the xinput_buffer, xoutput_buffer, and xnull classes, focusing on their interaction with various inputs and outputs. They verify the correct behavior of these classes when handling normal and edge cases, such as empty outputs, the correct invocation of callback functions, and the discarding of output by xnull.

Currently, these new test cases were executed and successfully passed.

Copy link
Contributor

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

}
}

TEST_SUITE("is_match_magics_manager")
Copy link

Choose a reason for hiding this comment

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

@brauliorivas if I understand correctly you can improve this test to check xmagics_manager::apply with line magic instead. it would std::regex_search %python same as is_match but also check if the magic can be successfully applied(and possibly record any breaking of the magic commands)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright! I'll change it

Copy link
Contributor Author

@brauliorivas brauliorivas Mar 12, 2024

Choose a reason for hiding this comment

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

Hey @maximusron I'm working on this test case as you said but I'm having trouble with the apply function. If you have time, some help would be handy!

@aaronj0
Copy link

aaronj0 commented Mar 11, 2024

@anutosh491 @JohanMabille could you take a look at this PR?
thanks!

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.78%. Comparing base (1524afd) to head (d6de68a).
Report is 7 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
- Coverage   49.32%   48.78%   -0.54%     
==========================================
  Files          13       15       +2     
  Lines         742      783      +41     
==========================================
+ Hits          366      382      +16     
- Misses        376      401      +25     

see 6 files with indirect coverage changes

see 6 files with indirect coverage changes

@vgvassilev
Copy link
Contributor

@brauliorivas, do you have any clue which part of the new changes breaks windows?

@brauliorivas
Copy link
Contributor Author

@brauliorivas, do you have any clue which part of the new changes breaks windows?

I'm going to debug and try to solve the Windows issue.

Copy link
Contributor

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

xcpp::xmagics_manager* magics = new xcpp::xmagics_manager();
manager.register_preamble(name, magics);

xcpp::xholder_preamble& result = manager.operator[](name);
Copy link
Contributor

@vgvassilev vgvassilev Mar 14, 2024

Choose a reason for hiding this comment

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

We should probably annotate with XEUS_CPP_API the xholder_preamble class to fix the Windows failure...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I've added this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @vgvassilev, thank you so much for the help, it worked!

Copy link
Contributor

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

@brauliorivas brauliorivas requested a review from vgvassilev March 15, 2024 17:39
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! However, I'd wait for @anutosh491's review.

@vgvassilev vgvassilev requested a review from anutosh491 March 15, 2024 17:41
@vgvassilev
Copy link
Contributor

LGTM! However, I'd wait for @anutosh491's review.

I'd like to move forward and rely on a post-commit review here.

@vgvassilev vgvassilev merged commit f33ae90 into compiler-research:main Mar 19, 2024
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.

4 participants