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

Expression tests and related issues #384

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

wti
Copy link

@wti wti commented Oct 8, 2023

As promised in #299, I filled in some of the expression tests, raising three kinds of issues:

Test helper

  • The top-level method actExp(..) is to replace XCTest assertion directly.
    It identifies the first text delta and prints expected and actual cleanly for copy/paste. But it is relatively ungainly and not isolated, and there are likely diffing libraries that would do a better job. It can merge and work as-is, but I can clean it up per your preferences. I used it only in the new tests.

Three kinds of test issues

  1. 3 tests fail due to JSON encoding differences, presumed due to locale differences
  • I flagged these as expected fails. In the expected locale, these should be reported as unexpected pass.
  • I can investigate whether JSONEncoder configuration will avoid locale-dependent behavior.
  1. testAsyncExpression3(): fails when there is no network
  • It's unclear if we should check and skip, or use a different async call that does not require a network.
  1. The test testIdentifierExprSyntax() now refers to a quoted keyword (`class`). Let me know if something else was intended.
  • The variable `class` is rendered as [-n-]{+cl+}a[-me-]{+ss+}. Is that correct?

Two implementation issues

  1. optional string has quotes around the entire optional: "Optional(\"0\")"
  2. inout variables are rendered constant (unusable) by macro expansion, resulting in compile errors.

Merging
Commits with aspects to investigate are marked DRAFT. Commits are designed to merge even if the issues above require changes (e.g., tests should continue to pass; the current behavior is accepted for now). Commits are also separated to isolate the issues above, to allow skipping a particular issue/commit. The goal was to enable the changes to be merged, with follow-on work scheduled in later tasks.

@wti
Copy link
Author

wti commented Oct 8, 2023

Sorry, I can't see the logs for the failing Bitrise or Test checks. I tested on macOS 14 with 5.9. (It fails as expected on Linux due to missing Foundation API's and objc support.)

@kishikawakatsumi
Copy link
Owner

Thank you for the PR! I'll check back later.

@wti
Copy link
Author

wti commented Oct 8, 2023

Sorry, please ignore the first commit to add .project to the .gitignore (cb6d4e...). (I've added that to my global configuration.)

@wti
Copy link
Author

wti commented Oct 14, 2023

Please let me know if you'd like me to rebase or merge the branch.

@kishikawakatsumi
Copy link
Owner

No worries. I just can't take my time.

@kishikawakatsumi
Copy link
Owner

Thank you very much. It looks good to me. All tests are passing in my local environment, so if there is a problem when I run it on CI, I will fix it on my end.

@kishikawakatsumi kishikawakatsumi self-requested a review October 15, 2023 15:49
@wti
Copy link
Author

wti commented Oct 18, 2023

Thank you for your patience.

I rebased off main, removed the unnecessary commit, and removed the expected failures per request.

fyi, four tests in AssertTests are failing for me:

  • testMagicLiteralExpression2
  • testInitializerExpression
  • testSelectorExpression1
  • testSelectorExpression2

(XCode 15.0, swift 5.9.0, M1 Studio)

If I discover a solution for inconsistent tests, I'll make a separate PR.

Thanks!

wti added 12 commits December 12, 2023 11:09
- literals: nil, floats, int, bool, array
- operators: binary, postfix, prefix, function call
- optional: nil, force-unwrap, chaining
- access: member, key-path
DRAFT: Need to verify test should target a keyword used as an identifier

DRAFT: need to verify expected output:

```
+++ [String] `class`
[-n-]{+cl+}a[-me-]{+ss+}
```
The assert macro expansion renders the inout variable constant,
which makes it unusable in the expression,
which causes the compile to fail.

This may be a bug or limitation of the macro or swift-syntax.
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.

2 participants