Conversation
I've split the main test file `test/abieos.test.js` into eight smaller, more focused test files based on the functionality they cover: - abi_management.test.js - serialization.test.js - deserialization.test.js - type_conversion.test.js - data_types.test.js - basic_types.test.js - eosio_specific_types.test.js - complex_types.test.js I've also consolidated a common test setup function `setupAbieos` into `test/test-helpers.js`. This function now handles the creation and configuration of the Abieos instance used in tests, including mock methods for tracking loaded ABIs. All new test files utilize this shared setup. The original `test/abieos.test.js` file has been removed. During the verification step, I updated some test assertions (primarily error message checks and a time string format) to accurately reflect the behavior of the underlying native Abieos library when accessed via the JavaScript wrapper. All tests pass after these changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @igorls, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request titled "Test suite splitting".
The primary goal of this PR is to refactor the existing test suite by breaking down the large, monolithic test file (test/) into multiple smaller, more focused files. This improves the organization, readability, and maintainability of the tests.
The changes involve deleting the single large test file and introducing several new files, each dedicated to testing a specific aspect of the abieos wrapper functionality, such as ABI management, serialization, deserialization, basic types, complex types, and EOSIO-specific types.
Additionally, the test helper functions in test/test-helpers.js have been updated to better support the new structure, including improvements to how Abieos instances are set up and how loaded ABIs are managed across potentially multiple test suites. A helper function for testing round-trip serialization/deserialization has also been introduced or refined.
Highlights
- Test Suite Refactoring: The main test file (
test/) has been split into 8 smaller, topic-specific test files. - Improved Organization: Tests are now organized by the specific functionality they cover (ABI management, serialization, different data types), making it easier to find and understand tests.
- Updated Test Helpers: The
test-helpers.jsfile was modified to enhance thesetupAbieosfunction for instance-specific state management and introduce/refine atestRoundTriphelper for common test patterns.
Changelog
Click here to see the changelog
- test/
- Deleted the single, large test file containing all wrapper tests.
- test/abi_management.test.js
- Added a new file containing tests specifically for ABI loading and management functions (
loadAbi,getLoadedAbis,clearLoadedAbis).
- Added a new file containing tests specifically for ABI loading and management functions (
- test/basic_types.test.js
- Added a new file containing tests for serialization and deserialization of various basic types (integers, floats, boolean, arrays, optionals).
- test/complex_types.test.js
- Added a new file containing tests for complex data structures including structs, optionals within structs, variants, and nested structures.
- test/data_types.test.js
- Added a new file containing tests for serialization and deserialization of a basic struct with common types.
- test/deserialization.test.js
- Added a new file containing tests specifically for the
hexTojsondeserialization function, including error cases.
- Added a new file containing tests specifically for the
- test/eosio_specific_types.test.js
- Added a new file containing tests for EOSIO-specific types like name, asset, time types, and public keys.
- test/serialization.test.js
- Added a new file containing tests specifically for the
jsonToHexserialization function, including error cases.
- Added a new file containing tests specifically for the
- test/test-helpers.js
- Modified the
setupAbieoshelper to manage loaded ABIs per instance and correctly bind original methods. - Added or refined the
testRoundTriphelper function to simplify writing serialization/deserialization tests. - Updated
assertThrowsusage or regex patterns in some tests (reflected in the new files).
- Modified the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
One test file, large and grand,
Split into many, close at hand.
Organized and neat,
A testing suite complete,
Easy to read, understand.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively splits a large, monolithic test suite into smaller, more manageable files, which is a significant improvement for maintainability and clarity. The updates to test-helpers.js, particularly the setupAbieos function, enhance test isolation by ensuring that each test suite (or beforeEach block) operates with a cleaner state for loaded ABIs. This is a commendable refactoring effort.
My main concern revolves around the changes to error message assertions in serialization.test.js, which now expect a more generic error. This seems to stem from a specific error handling pattern in the jsonToHex method within the JavaScript wrapper (dist/abieos.js). While the tests correctly reflect the current behavior of this wrapper, it's worth discussing if this generalization of error messages is intended and optimal, as it might mask more specific underlying causes of failures.
Overall, the structural changes are excellent. Addressing the error handling clarification would further solidify the quality of the test suite.
Summary of Findings
- Test Suite Refactoring: The primary goal of splitting the monolithic test file into multiple, focused test suites has been successfully achieved. This greatly enhances the maintainability and readability of the tests.
- Test Helper Improvements: The
test-helpers.jsfile, particularly thesetupAbieosfunction, has been improved to provide better test isolation by managing loaded ABI state more effectively per test setup. - Error Message Specificity in Serialization Tests: Tests for
jsonToHexinserialization.test.jsnow assert against a generic error message (/failed to parse data/i). This is due to thejsonToHexJavaScript wrapper's handling of a "PARSING_ERROR" string from the native layer. This change reduces the specificity of error checking, which might make debugging harder. It's recommended to review if more specific error propagation is feasible and desirable.
Merge Readiness
The refactoring of the test suite is a significant improvement. However, there's a medium-severity concern regarding the generalization of error messages in the serialization tests. It's recommended to address this concern by clarifying the intended error handling strategy for jsonToHex before merging. As a reviewer, I am not authorized to approve pull requests; please ensure further review and approval from authorized maintainers after addressing the feedback.
| test('should throw if ABI for contract is not loaded', () => { | ||
| const actionData = { from: "a", to: "b", quantity: "1.0 EOS", memo: "m" }; | ||
| assertThrows( | ||
| () => abieos.jsonToHex("unknown.contract", "transfer", actionData), | ||
| /failed to parse data/i, // Updated regex | ||
| 'Should throw if ABI is not loaded' | ||
| ); | ||
| }); | ||
|
|
||
| test('should throw if type is not found in ABI', () => { | ||
| const actionData = { from: "a", to: "b", quantity: "1.0 EOS", memo: "m" }; | ||
| // While setupAbieos.getType might throw a specific error, jsonToHex's C++ part might fail first, | ||
| // leading to the generic wrapper error. | ||
| assertThrows( | ||
| () => abieos.jsonToHex(contractAccount, "unknown_type", actionData), | ||
| /failed to parse data/i, // Updated regex | ||
| 'Should throw if type is not found' | ||
| ); | ||
| }); | ||
|
|
||
| test('should throw for data with missing required fields', () => { | ||
| const actionData = { | ||
| from: "alice", | ||
| quantity: "1.0000 EOS", | ||
| memo: "test transfer" | ||
| }; | ||
| assertThrows( | ||
| () => abieos.jsonToHex(contractAccount, "transfer", actionData), | ||
| /failed to parse data/i, // Updated regex | ||
| 'Should throw for missing required fields' | ||
| ); | ||
| }); | ||
|
|
||
| test('should throw for data with incorrect types', () => { | ||
| const actionData = { | ||
| from: "alice", | ||
| to: "bob", | ||
| quantity: 12345, | ||
| memo: "test transfer" | ||
| }; | ||
| assertThrows( | ||
| () => abieos.jsonToHex(contractAccount, "transfer", actionData), | ||
| /failed to parse data/i, // Updated regex | ||
| 'Should throw for incorrect data types' | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The error message assertions in this test suite (e.g., for contract not loaded, type not found, missing fields, incorrect types) have been updated to expect a generic /failed to parse data/i error. This aligns with the current jsonToHex implementation in dist/abieos.js which throws new Error("failed to parse data") if the native call returns the string "PARSING_ERROR".
Previously, the tests for these scenarios asserted more specific error messages, potentially including details from the underlying native errors (e.g., contract ... not loaded, Unknown type ..., Expected field ...).
Could you clarify if this generalization of error messages in jsonToHex (and consequently in these tests) is an intentional design choice? While the tests now match the wrapper's behavior, relying on a generic error message might make it harder to diagnose the specific root cause of a serialization failure. If the native layer provides more specific error details, it might be beneficial for the JavaScript wrapper to propagate them or for the tests to assert against those more granular errors if possible.
For example, the hexToJson wrapper (and its tests in deserialization.test.js) seems to retain more specific error messages. Understanding the rationale for the difference in error reporting between jsonToHex and hexToJson would be helpful.
1 participant
No description provided.