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

test: replicate incorrect bundling and missing assertions part of #1323 #1389

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

francocm
Copy link

Description

Related issue(s)

Replicates #1323

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

Copy link
Member

@peter-rr peter-rr left a comment

Choose a reason for hiding this comment

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

Thanks for raising this draft! I left some comments :)

test/integration/bundle/bundle.test.ts Outdated Show resolved Hide resolved
test/integration/bundle/bundle.test.ts Outdated Show resolved Hide resolved
test/integration/bundle/bundle.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@peter-rr peter-rr left a comment

Choose a reason for hiding this comment

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

Left some comments 😄

test/integration/bundle/bundle.test.ts Outdated Show resolved Hide resolved
test/integration/bundle/bundle.test.ts Outdated Show resolved Hide resolved
test/integration/bundle/bundle.test.ts Outdated Show resolved Hide resolved
@francocm
Copy link
Author

francocm commented May 9, 2024

I've pulled and synced the changes from master branch into here, and the good news is after #1415 (@aeworxet), the original issue mentioned in #1323 seems solved.

However, the corrected tests are now showing 1 new issue coming probably from the bundler.

The bundler, since 0.5.0, makes it clear that objects must be dereferenced to the maximum extent possible. At least that's how I'm interpreting this.

However, when running the tests, for ASyncAPI v3.0.0 docs, it's leaving the following behind:

components:
  messages:
    TestMessage:
      payload:
        type: string
      x-origin: '#/components/messages/TestMessage'

This happens for both x-origin use and without. In case of x-origin, it does get referenced through x-origin itself, but I believe this shouldn't matter. In fact, with x-origin: false, then there's no references at all, yet still present. The content gets dereferenced successfully, just not cleaned up.

Right now this is the only thing keeping it failing.

@peter-rr @Amzani @Souvikns @Shurtu-gal any thoughts how you'd like to proceed?

Options I can think of:

  1. Raise it as a separate bug, and for now I remove the 'offending' part from the test comparison, and this gets solved.
  2. It's not a bug (please verify my interpretation), so will simply fix the test and this gets solved.
  3. Get it resolved in bundler and keep this PR waiting until that gets fixed.

Thanks!

@aeworxet
Copy link
Contributor

@francocm
I am afraid this is not an issue of Bundler, because Bundler doesn't have such complex logic that could lead to such errors. JSON schema is simply transferred to @apidevtools/json-schema-ref-parser with several conditions set, so this issue is most probably the doing of that package.

I have traced the issue up to the crawl() function in @apidevtools/json-schema-ref-parser and opened an issue in its repository.

@francocm francocm marked this pull request as ready for review May 13, 2024 11:09
@francocm
Copy link
Author

@aeworxet

Thanks for the analysis.

Worth adding that although this component is ignored, when x-origin: true is used, it's still being updated, as in that scenario, the x-origin: '#/components/messages/TestMessage' gets added, pointing to itself.

I have updated my PR to consider this outcome as expected in the tests, since the scope of this PR is on the issue raised in #1323 and:

  1. assertions that replicated the original problem
  2. assertions that were not actually being checked on existing tests
  3. will now act as better regression test library for future changes

Probably anything else needs to be done in a separate place, and could be tracked in a new issue + linked to the APIDevTools/json-schema-ref-parser#349 you raised.

@peter-rr I've updated the PR with feedback, tests should be passing, and it's no longer a draft. Kindly let me know your thoughts at this stage.

Thanks 🙂

@peter-rr
Copy link
Member

/update

Copy link
Member

@peter-rr peter-rr left a comment

Choose a reason for hiding this comment

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

Hey @francocm, great work! 😄
The changes you have applied look good to me. However, all the tests related to bundle command are not passing since I'm getting this error when executing the command for any test case:

Error: Unable to bundle specification file of different major versions

I've updated your branch with the latest changes from master but can't make the tests pass. Could you please try to run the tests again after updating your branch to check the result?

Also we're getting this error on the CI workflow related to linter. You can fix it easily by running npm run lint:fix

@peter-rr
Copy link
Member

peter-rr commented May 14, 2024

Probably anything else needs to be done in a separate place, and could be tracked in a new issue + linked to the APIDevTools/json-schema-ref-parser#349 you raised.

Yeah, I agree with this 👍
Once the new tests are passing I think we could proceed to close the bug related #1323

@peter-rr
Copy link
Member

/update

@francocm
Copy link
Author

Hi @peter-rr,

Thanks for following up. I've run npm run lint:fix and pushed it back.

Running npm run test seems to be running fine here?

  129 passing (45s)

Full log:

➜  asyncapi-cli git:(fix/1323/bundleNotBundlingExternalFiles) ✗ npm run test    
> @asyncapi/[email protected] pretest
> npm run build


> @asyncapi/[email protected] build
> rimraf lib && node scripts/fetch-asyncapi-example.js && tsc && oclif manifest && echo "Build Completed"

Fetched ZIP file
Unzipped all examples from zip
 ›   Warning: oclif update available from 4.5.0 to 4.8.8.
wrote manifest to /Users/<username_redacted>/workspace/asyncapi-cli/oclif.manifest.json
Build Completed

> @asyncapi/[email protected] test
> npm run test:unit


> @asyncapi/[email protected] test:unit
> cross-env NODE_ENV=development TEST=1 CUSTOM_CONTEXT_FILENAME="test.asyncapi-cli" CUSTOM_CONTEXT_FILE_LOCATION="" nyc --extension .ts mocha --require ts-node/register --require test/helpers/init.js --reporter spec --timeout 100000 "test/**/*.test.ts"



  bundle
    ✔ should throw error message if the file path is wrong

  bundle spec v3

  config:analytics
    with disable flag
    with enable flag
    with no flags
    with status flag

  config
    config:versions

  config:context, positive scenario
    config:context:current
    config:context:list
    config:context:add
    config:context:add
    config:context:edit
    config:context:use
    config:context:remove
    config:context:init
    config:context:init
    config:context:init
    config:context:init

  config:context, negative scenario
    config:context:add
    config:context:add
    config:context:add
    config:context:add

  config:context, negative scenario
    config:context:list

  convert
    with file paths
    with no arguments
    with no context file
    with target-version flag
    with output flag

  diff
    should handle AsyncAPI v3 document correctly
    with file paths, and there are no difference between the files
    yaml output: with file paths, and there are no difference between the files
    with file paths, and getting all changes
    with file paths, and getting breaking changes
    with file paths, and getting non-breaking changes
    with file paths, and getting unclassified changes
    with file paths, and getting all changes, passing flag
    YAML output, getting all changes
    should show error on breaking changes
    Markdown output with subtype as json, getting all changes
    Markdown output with subtype as yaml, getting all changes
    Other output with markdownSubtype flag provided, check for warning
    with logging diagnostics
    with custom standard file
    should throw error for invalid override path
    should throw error for invalid override json

  template
    should handle AsyncAPI v3 document correctly
    git clash
Generation failed
      ✔ should throw error if output folder is in a git repository (205ms)
    custom params
    disable-hooks
    debug
    no-overwrite
    should install template
Generator Error: Installation failed
    map-base-url

  models
    should handle AsyncAPI v3 document correctly
    for TypeScript
    for JavaScript
    for Python
    for Rust
    for C#
    for C++
    for Java
    for Go
    for Kotlin
    for Dart
    for PHP
    with logging diagnostics

  new
    create new file
    when asyncapi file already exists

  new glee
    creation of new project is successful
    when new project name already exists

  optimize
    no optimization needed
    with no arguments
    with no context file
    no-tty flag
    interactive terminal
    error if the asyncapi file is invalid

  validate
    with file paths
    with context names
    with no arguments
    with no context file
    with --log-diagnostics flag
    with --diagnostics-format flag
    with --fail-severity flag


  129 passing (45s)

-----------------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------
File                         | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                                                                            
-----------------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------
All files                    |   59.91 |     40.2 |   57.22 |   60.09 |                                                                                              
 lib                         |    79.7 |    51.54 |    87.5 |    79.5 |                                                                                              
  base.js                    |    83.9 |    56.75 |     100 |    83.9 | 63-64,94-95,132,140-153                                                                      
  flags.js                   |     100 |      100 |     100 |     100 |                                                                                              
  globals.js                 |    38.7 |        0 |       0 |    38.7 | 13-15,20-45                                                                                  
  parser.js                  |   89.74 |    60.97 |     100 |   89.47 | 96-98,100-102,121,124                                                                        
 lib/commands                |    90.9 |    73.71 |     100 |   90.82 |                                                                                              
  bundle.js                  |   86.48 |    55.55 |     100 |   86.48 | 26-30,57-58                                                                                  
  convert.js                 |   94.87 |    77.77 |     100 |   94.87 | 54,60                                                                                        
  diff.js                    |   88.49 |    78.33 |     100 |   88.49 | 49,71,90,107,117,138,182-184,186,194,229,256                                                 
  optimize.js                |   92.37 |     73.4 |     100 |   92.17 | 87,94-104,113                                                                                
  validate.js                |   95.65 |       75 |     100 |   95.65 | 18                                                                                           
 lib/commands/config         |   86.88 |    76.47 |     100 |   86.44 |                                                                                              
  analytics.js               |   80.55 |    69.23 |     100 |   80.55 | 35-48                                                                                        
  versions.js                |      96 |      100 |     100 |   95.65 | 26                                                                                           
 lib/commands/config/context |   75.64 |    17.77 |     100 |   75.64 |                                                                                              
  add.js                     |      84 |       50 |     100 |      84 | 19-20,25-26                                                                                  
  current.js                 |   58.33 |     7.69 |     100 |   58.33 | 16-29                                                                                        
  edit.js                    |   70.83 |        0 |     100 |   70.83 | 19-27                                                                                        
  init.js                    |     100 |      100 |     100 |     100 |                                                                                              
  list.js                    |   86.36 |       50 |     100 |   86.36 | 14-15,28                                                                                     
  remove.js                  |   69.56 |        0 |     100 |   69.56 | 18-26                                                                                        
  use.js                     |   69.56 |        0 |     100 |   69.56 | 18-26                                                                                        
 lib/commands/generate       |   67.05 |    57.79 |   58.13 |   67.15 |                                                                                              
  fromTemplate.js            |   59.67 |    44.73 |      60 |   59.56 | 39,49-68,119-120,130-139,145,148,155-163,169,187-189,195-196,207,222,225,241,247,258,277-339 
  models.js                  |   75.79 |    70.51 |   53.84 |   76.12 | 37-41,49-51,59,68,170,173,176,214,235-273                                                    
 lib/commands/new            |   49.47 |    20.77 |   45.83 |   49.47 |                                                                                              
  file.js                    |   55.17 |    24.48 |   66.66 |   55.17 | 33,39-113,122,130-131,143                                                                    
  glee.js                    |   42.57 |    14.28 |   33.33 |   42.42 | 39-117,130,133-134,144-152,163                                                               
  index.js                   |     100 |      100 |     100 |     100 |                                                                                              
 lib/errors                  |   65.81 |    31.25 |   65.21 |   66.07 |                                                                                              
  context-error.js           |   83.78 |      100 |   69.23 |    87.5 | 60-61,67-68                                                                                  
  diff-error.js              |     100 |      100 |     100 |     100 |                                                                                              
  generator-error.js         |     100 |      100 |     100 |     100 |                                                                                              
  specification-file.js      |   54.83 |    58.33 |      25 |   54.83 | 7-18,25-26,41-42,45-46,49-50                                                                 
  validation-error.js        |   31.03 |       15 |      50 |   31.03 | 8,14,19-48                                                                                   
 lib/models                  |   69.66 |     60.9 |   70.51 |    69.6 |                                                                                              
  Context.js                 |   80.28 |    61.66 |     100 |   80.14 | 74-75,88,107,133,136,150,164,167,182,207,227,245-267,291-294                                 
  SpecificationFile.js       |   82.64 |    72.13 |   84.37 |   82.35 | 43,56,75,86-90,99-106,117,120,123,140,143,159,188-193,220                                    
  Studio.js                  |   25.71 |        0 |       0 |   26.08 | 20-101,106-123                                                                               
 lib/utils                   |   12.67 |        0 |       0 |   13.04 |                                                                                              
  generator.js               |   12.67 |        0 |       0 |   13.04 | 11-13,19-149                                                                                 
 src                         |   14.92 |        0 |       0 |   15.15 |                                                                                              
  base.ts                    |   14.92 |        0 |       0 |   15.15 | 20-132                                                                                       
 src/commands                |   21.42 |     4.25 |   22.22 |   22.01 |                                                                                              
  optimize.ts                |   21.42 |     4.25 |   22.22 |   22.01 | 27,49-210                                                                                    
 src/errors                  |   20.87 |        0 |       0 |   20.87 |                                                                                              
  context-error.ts           |      40 |      100 |       0 |      40 | 11,13,15,17,19-24,30-31,37-38,44-45,51-52,58-59,65-66,72-73                                  
  specification-file.ts      |   13.79 |        0 |       0 |   13.79 | 4-5,11-15,22-23,30-54                                                                        
  validation-error.ts        |     3.7 |        0 |       0 |     3.7 | 11-57                                                                                        
 src/models                  |   23.82 |     7.43 |   10.52 |   24.22 |                                                                                              
  Context.ts                 |   27.77 |       15 |   13.33 |    27.2 | 85-121,125-139,143-153,157-170,174-182,189-200,208-215,223-260,280-309,335-340,351-358,368   
  SpecificationFile.ts       |   19.26 |        0 |    8.69 |   20.58 | 27-97,113,125-155,160-171,176-180,185-223                                                    
-----------------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------

> @asyncapi/[email protected] posttest
> rimraf ./test.asyncapi-cli

➜  asyncapi-cli git:(fix/1323/bundleNotBundlingExternalFiles) ✗ 

@asyncapi-bot
Copy link
Contributor

Hello, @francocm! 👋🏼

    I'm 🧞🧞🧞 Genie 🧞🧞🧞 from the magic lamp. Looks like somebody needs a hand!

    At the moment the following comments are supported in pull requests:

    - `/please-take-a-look` or `/ptal` - This comment will add a comment to the PR asking for attention from the reviewrs who have not reviewed the PR yet.
    - `/ready-to-merge` or `/rtm` - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
    - `/do-not-merge` or `/dnm` - This comment will block automerging even if all conditions are met and ready-to-merge label is added
    - `/autoupdate` or `/au` - This comment will add `autoupdate` label to the PR and keeps your PR up-to-date to the target branch's future changes. Unless there is a merge conflict or it is a draft PR. (Currently only works for upstream branches.)
    - `/update` or `/u` - This comment will update the PR with the latest changes from the target branch. Unless there is a merge conflict or it is a draft PR. NOTE: this only updates the PR once, so if you need to update again, you need to call the command again.

@francocm
Copy link
Author

Just realised, if it helps I'm using Node 20:

➜  asyncapi-cli git:(fix/1323/bundleNotBundlingExternalFiles) ✗ node --version
v20.12.2
➜  asyncapi-cli git:(fix/1323/bundleNotBundlingExternalFiles) ✗ npm --version 
10.5.0
➜  asyncapi-cli git:(fix/1323/bundleNotBundlingExternalFiles) ✗ 

@peter-rr
Copy link
Member

Just realised, if it helps I'm using Node 20

Thanks @francocm for the tip 👍 I've tried with v20 but still getting the errors. Also now I'm getting many errors during the building process after updating CLI to the latest version 1.14.0. Even working on master branch. Let me investigate why I'm getting those results 🤔 and I'll get back to you.

@aeworxet
Copy link
Contributor

@peter-rr
Can you please try cloning, npm-installing, and running your tests from zero? Sometimes there are manual changes you completely forgot you made, and they are not displayed with git status because files are in .gitignore.
Knowing the logic of Bundler, it explicitly compares the integer part of the asyncapi property of the second AsyncAPI Document with the integer part of the asyncapi property of the first AsyncAPI Document, and it is very hard to compare two integers incorrectly if no changes are introduced somewhere else in the code.

@peter-rr
Copy link
Member

Thanks @aeworxet 😄 I just needed npm-installing to include the latest version of @oclif/core dependency. Everything working fine now.

@peter-rr
Copy link
Member

@francocm
LGTM 👍 All tests passing after the latest changes applied.

Now this PR needs to be reviewed by the codeowners before being merged.

@peter-rr
Copy link
Member

/ptal

@asyncapi-bot
Copy link
Contributor

@Amzani @Souvikns @Shurtu-gal Please take a look at this PR. Thanks! 👋

@peter-rr
Copy link
Member

/update

@Shurtu-gal
Copy link
Collaborator

/update

Copy link

sonarcloud bot commented May 17, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Collaborator

Choose a reason for hiding this comment

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

@francocm could you add the spec files in test/fixtures as can be seen for others.

Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Sep 15, 2024
@Shurtu-gal
Copy link
Collaborator

still relevant

@francocm are you still working on this?

@github-actions github-actions bot removed the stale label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

5 participants