Skip to content

add elixir workflow #21215

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

Merged

Conversation

efcasado
Copy link
Contributor

@efcasado efcasado commented May 4, 2025

Description

Contributions to #21214 to unblock the work done by @wing328.

This PR includes the following fixes:

  • References to Jason: Jason was replaced by the built-in JSON module in [feat][elixir] use elixir 1.18 built-in json module #21039 but the tests were not updated accordingly.
  • Tesla does not follow redirects by default: It seems petstore.swagger.io used to be accessible on using plain http on port 80; but it seems things changed at some point and the tests were not updated accordingly.
  • Temporarily disabled type-casting for dates: It seems type casting of date fields broke in [fix][elixir] wrong typespec generation for all-of with single ref #21139. Since the fix requires a change in the Elixir generator, the change has been extracted to its own pull request, ie. [fix][elixir] type casting for dates not working #21216.
  • Retry failing tests: A lot has changed in petstore.swagger.io since the last time these tests were executed. Tests that require an answer from the remote server are failing because there seem to be multiple instances of the (remote) Petstore and not all instances return the valid response. The issue has been mitigated by re-trying the failing step a number of times. This, however, is not deterministic and we need to find a better way to test things.

@wing328
Copy link
Member

wing328 commented May 4, 2025

Error: test/format_test.exs:5

can you please have a look at this one too?

for the petstore (http request, response test), please ignore those for the time being

@efcasado
Copy link
Contributor Author

efcasado commented May 4, 2025

Error: test/format_test.exs:5

can you please have a look at this one too?

Yes, these are the ones I am looking at at the moment.

It was introduced in #21139 (you can see the actual change here). I should have a fix ready soon.

@efcasado
Copy link
Contributor Author

efcasado commented May 4, 2025

I reverted the change to config/config.exs since I realized is a generated file 😓

@efcasado efcasado force-pushed the add-elixir-workflow branch from aa00f87 to 19c7c53 Compare May 4, 2025 20:17
@efcasado efcasado changed the title Add elixir workflow add elixir workflow May 4, 2025
@efcasado
Copy link
Contributor Author

efcasado commented May 4, 2025

@wing328, I believe the changes are ready to be merged into your branch 🙏

@wing328
Copy link
Member

wing328 commented May 5, 2025

3) test decode all properties (not nil) (FormatTest)
Error:      test/format_test.exs:5
     Assertion with == failed
     code:  assert %FormatTest{
              integer: 1,
              int32: 2,
              int64: 3,
              number: 4.1,
              float: 5.2,
              double: 6.3,
              decimal: "7.4",
              string: "Hello world!",
              byte: "U3dhZ2dlciByb2Nrcw==",
              binary: <<1, 2, 3>>,
              date: "2013-10-20",
              dateTime: "2013-10-20T19:20:30+01:00",
              uuid: "3fa85f64-5717-4562-b3fc-2c963f66afa6",
              password: "green?horse",
              pattern_with_digits: "1234567890",
              pattern_with_digits_and_delimiter: "Image_01"
            }
            |> FormatTest.decode() == %FormatTest{
              integer: 1,
              int32: 2,
              int64: 3,
              number: 4.1,
              float: 5.2,
              double: 6.3,
              decimal: "7.4",
              string: "Hello world!",
              byte: "U3dhZ2dlciByb2Nrcw==",
              binary: <<1, 2, 3>>,
              date: ~D[[2013-10-20],
              dateTime: ~U[2013-10-20T18:20:30Z],
              uuid: "3fa85f64-5717-4562-b3fc-2c963f66afa6",
              password: "green?horse",
              pattern_with_digits: "1234567890",
              pattern_with_digits_and_delimiter: "Image_01"
            }

what about updating the tests with date/datetime accordingly as these are the causing the test failure?

@efcasado
Copy link
Contributor Author

efcasado commented May 5, 2025

We could do that, but that would only hide the actual issue and change the semantics of the test - right?

The test is failing because the decoding step is not automatically casting strings to Date.t and DateType.t.

This PR fixes the issue caught by those tests. I went for a separate PR for that change because it requires a bug fix in the Elixir generator as opposed to just adjusting the tests.

Alternatively, we could adjust the failing tests here (even it breaks their semantics) in order to get a green build; and use #21216 to re-introduce the old behavior and re-adjust the formatting tests again. What do you think?

@efcasado
Copy link
Contributor Author

efcasado commented May 5, 2025

@wing328, all tests are passing now

mix test
Running ExUnit with seed: 636492, max_cases: 40

............
Finished in 2.3 seconds (2.3s async, 0.00s sync)
12 tests, 0 failures

There are a number of issues with how the Elixir generator approaches testing. For now, I focused on getting a green build to unblock the work you are doing, but I feel the existing tests need to be reviewed and improved. I left some basic observations in the description of this PR.

@wing328 wing328 merged commit 0a5cadd into OpenAPITools:add-elixir-workflow May 5, 2025
8 checks passed
@wing328
Copy link
Member

wing328 commented May 5, 2025

agreed there are rooms for improvements

some other client generators have moved to Echo API server instead of Petstore server for testing.

Elixir client may also consider adding Echo API server tests:

https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests#echo-server

@wing328 wing328 added this to the 7.14.0 milestone May 5, 2025
wing328 added a commit that referenced this pull request May 5, 2025
* add elixir workflow

* update

* fix

* add elixir workflow (#21215)

* update tests to use built-in json module instead of jason

* update base_url

* temporarily disable type-casting for dates

* retry failing tests

* update spec to use localhost

* add petsore local server to workflow

---------

Co-authored-by: Enrique Fernández <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants