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

fix upload files when array items are type=string format=binary #6040

Merged
merged 5 commits into from
Jun 2, 2020

Conversation

JustMik
Copy link
Contributor

@JustMik JustMik commented May 28, 2020

Description

I changed condition of file item for type array properties.
An item is a file if:

  • type=file
  • type=string and format=binary

Motivation and Context

This solve multiple files upload, as shown in attached screenshots.

How Has This Been Tested?

Screenshots (if appropriate):

Before fix:
before

After fix:
after

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@JustMik JustMik changed the title fix upload file when array items are type=string format=binary fix upload files when array items are type=string format=binary May 28, 2020
# Conflicts:
#	dist/swagger-ui-bundle.js
#	dist/swagger-ui-bundle.js.map
#	dist/swagger-ui.js
#	dist/swagger-ui.js.map
@tim-lai
Copy link
Contributor

tim-lai commented May 29, 2020

@JustMik please remove the dist/* from this PR. otherwise, looks good to me. Thanks!

@tim-lai
Copy link
Contributor

tim-lai commented Jun 1, 2020

please build

@JustMik
Copy link
Contributor Author

JustMik commented Jun 1, 2020

Hi @tim-lai .. Sorry but is my first contribution and i've some difficulty... Before first pull request i ran locally tests and they finished successfully... For remove dist/* , i've aligned my branch to head but test fails (bugs/4641.js)(locally and on github pipeline).. Can you give me some advice please?

@tim-lai
Copy link
Contributor

tim-lai commented Jun 1, 2020

@JustMik Thanks for the followup. There are 2 items occurring.

First, bugs/4641 is a known flaky test, and the recent attempt to resolve was insufficient. I will be working on this again in the near term.

Second, your updated PR passes test on my local system. So that's good news from the PR perspective. However, can you provide config details on your local system? In the meantime, you can try npm i again, as there has been dependency changes over the course of recent releases.

@JustMik
Copy link
Contributor Author

JustMik commented Jun 1, 2020

@tim-lai
I've install dependencies npm i, build npm run build and test npm run test but i have following test error on first step of bugs/4641:

CypressError: Timed out retrying: cy.its() errored because your subject is: 'null'. You cannot access any properties such as 'request' on a 'null' value.

I'm working on Osx 10.15.5, npm 6.14.3..

Thank you for your support and let me know if you need other info.

@tim-lai
Copy link
Contributor

tim-lai commented Jun 1, 2020

@JustMik Thanks for the info. What version of node are you using? Also, can you copy/paste the debug log here?

@JustMik
Copy link
Contributor Author

JustMik commented Jun 1, 2020

@tim-lai I'm using node 13.11.0.
Following the debug logs. If you need all logs, i can attach them.

────────────────────────────────────────────────────────────────────────────────────────────────────

Running: bugs/4641.js (2 of 29)
spec.js:39
spec.js:39 #4641: The Logout button in Authorize popup not clearing API Key
spec.js:74 1) should include the given api key in requests
spec.js:68 ✓ should not remember the previous auth value when you logout and reauthorise (2865ms)
spec.js:68 ✓ should only forget the value of the auth the user logged out from (3395ms)
base.js:310 2 passing (16s)
base.js:326 1 failing
base.js:221 1) #4641: The Logout button in Authorize popup not clearing API Key should include the given api key in requests:
CypressError: Timed out retrying: cy.its() errored because your subject is: 'null'. You cannot access any properties such as 'request' on a 'null' value.

If you expect your subject to be 'null', then add an assertion such as:

cy.wrap(null).should('be.null')
at Object.cypressErr (http://localhost:3230/__cypress/runner/cypress_runner.js:86207:11)
at Object.throwErr (http://localhost:3230/__cypress/runner/cypress_runner.js:86162:18)
at Object.throwErrByPath (http://localhost:3230/__cypress/runner/cypress_runner.js:86194:17)
at retry (http://localhost:3230/__cypress/runner/cypress_runner.js:76849:16)
at onFailFn (http://localhost:3230/__cypress/runner/cypress_runner.js:65634:16)
at tryCatcher (http://localhost:3230/__cypress/runner/cypress_runner.js:120203:23)
at Promise._settlePromiseFromHandler (http://localhost:3230/__cypress/runner/cypress_runner.js:118139:31)
at Promise._settlePromise (http://localhost:3230/__cypress/runner/cypress_runner.js:118196:18)
at Promise._settlePromise0 (http://localhost:3230/__cypress/runner/cypress_runner.js:118241:10)
at Promise._settlePromises (http://localhost:3230/__cypress/runner/cypress_runner.js:118316:18)
at Async../node_modules/bluebird/js/release/async.js.Async._drainQueue (http://localhost:3230/__cypress/runner/cypress_runner.js:114928:16)
at Async../node_modules/bluebird/js/release/async.js.Async._drainQueues (http://localhost:3230/__cypress/runner/cypress_runner.js:114938:10)
at Async.drainQueues (http://localhost:3230/__cypress/runner/cypress_runner.js:114812:14)
run.js:732
terminal.js:195 (Results)
run.js:765
run.js:766 ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Tests: 3 │
│ Passing: 2 │
│ Failing: 1 │
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 1 │
│ Video: false │
│ Duration: 15 seconds │
│ Spec Ran: bugs/4641.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
run.js:767
run.js:771
terminal.js:195 (Screenshots)
run.js:775
run.js:800 - /Users/justin/Workspaces/Opensource/swagger-ui/test/e2e-cypress/screenshots/bugs (1280x720)
/4641.js/#4641 The Logout button in Authorize popup not clearing API Key -- shou
ld include the given api key in requests (failed).png
run.js:802

@tim-lai
Copy link
Contributor

tim-lai commented Jun 1, 2020

@JustMik I merged a new fix for test bugs/4641. Please merge again with master, and let me know how it goes for you on local. Thanks!

@JustMik
Copy link
Contributor Author

JustMik commented Jun 1, 2020

@tim-lai i merged from master but the first step of bug/4641 keeps failing.
From your local it working?

@tim-lai
Copy link
Contributor

tim-lai commented Jun 1, 2020

@JustMik hmm. Does the error log look the same? I'm curious, can you try updating Cypress version to v3.8.3 in package.json. Btw, I appreciate your assistance and time in troubleshooting.

Yes, it's working for my local, though bugs/4641 has always worked for me. I'm on Node v13.9. This test also periodically fails on Node v10.x. So I don't think it's Node.

@tim-lai tim-lai merged commit edb932b into swagger-api:master Jun 2, 2020
@tim-lai
Copy link
Contributor

tim-lai commented Jun 2, 2020

@JustMik an update: I made another change to the bugs/4641 test. I am able to update and merge your PR successfully. Thank you much for your contribution! If you have another few minutes, I would like to know if you are now able run and pass all tests on your local system.

@JustMik
Copy link
Contributor Author

JustMik commented Jun 2, 2020

Now the test run successfully on my local. Thank you @tim-lai.

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