Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move interop tests to the public interface test project #1767

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

BhaaLseN
Copy link
Contributor

Most of those interop tests illustrate how Jint can interact with objects from popular libraries such as Newtonsoft.Json, System.Text.Json and dynamic objects such as ExpandoObject.

Because this can be a valuable reference for Jint users, they should not depend on any internals (which would be off limits for callers.) See #1766 for my endeavor on (successfully) integrating STJ.

This pretty much only moves the files over to the other project and updates the one location where internal code was used. Other tests like Newtonsoft and Dynamic didn't really use any internals, so it was just the namespace update there.

Initially, I moved over all tests that began with InteropTests and quickly realized how most of them don't really mean interop in the sense of someone external trying to interface with it. At least I got stopped by various internal methods on ObjectWrapper, ObjectInstance etc. that I simply cannot justify changing just to move the tests.

Let me know if theres more (or possibly also less) you want included with this PR.

most of those interop tests illustrate how to interact with objects from
popular libraries such as Newtonsoft.Json, System.Text.Json and dynamic
objects such as ExpandoObject.

because this can be a valuable reference for Jint users, they should not
depend on any internals (which would be off limits for callers.)
@BhaaLseN BhaaLseN changed the title Move interop tests to the public interfacetest project Move interop tests to the public interface test project Jan 30, 2024
Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

A great improvement, thank you!

@lahma lahma merged commit 24c674f into sebastienros:main Jan 30, 2024
3 checks passed
@BhaaLseN BhaaLseN deleted the public-interop-tests branch January 30, 2024 19:35
@hyzx86
Copy link
Contributor

hyzx86 commented Mar 12, 2024

Hello @BhaaLseN , thanks for the PR, but I find that false for Boolean values doesn't seem to handle as expected

        const string Json = """
        {
            "booleanValue":false,
            "trueValue":true,
            "employees": {
                "type": "array",
                "value": [
                    {
                        "firstName": "John",
                        "lastName": "Doe"
                    },
                    {
                        "firstName": "Jane",
                        "lastName": "Doe"
                    }
                ]
            }
        }
        """;

// ...other code..... 
        Assert.True(engine.Evaluate("variables.booleanValue==false").AsBoolean());

@BhaaLseN
Copy link
Contributor Author

BhaaLseN commented Mar 12, 2024

I just moved the existing tests, I didn't make any functional changes. You should probably check Discussions and/or open an Issue instead.

Edit: Ah well, I didn't look at the links. Guess you already did.

@sebastienros
Copy link
Owner

Check this PR #1802, it passes but might not be the best impl.

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.

None yet

4 participants