Skip to content

import/export: Improve error handling, promisify#4957

Merged
JohnMcLear merged 20 commits intodevelopfrom
rhansen-export-errors
Mar 18, 2021
Merged

import/export: Improve error handling, promisify#4957
JohnMcLear merged 20 commits intodevelopfrom
rhansen-export-errors

Conversation

@rhansen
Copy link
Copy Markdown
Member

@rhansen rhansen commented Mar 17, 2021

Multiple commits:

  • tests: Increase import/export test timeouts
  • tests: Promisify import/export tests
  • tests: Use assert to simplify import/export tests
  • import/export: Delete unnecessary comments
  • Abiword: Reset stdout buffer when starting abiword
  • ExportHandler: Replace unnecessary exception with return
  • import/export: Use Error objects for errors, not strings
  • ExportHandler: Pass the error unmodified
  • import/export: Spelling fix: "convertor" -> "converter"
  • import/export: On export error return 500 instead of crashing
  • Abiword: Don't call the callback if null
  • Abiword: Reduce log spam
  • Abiword: Fix logging of conversion failure
  • Abiword: Use the async-provided callback to signal errors
  • LibreOffice: Add missing fileExtension property on intermediate step
  • LibreOffice: Use consistent intermediate filename
  • LibreOffice: Use async.series to properly handle conversion errors
  • LibreOffice: Use the async-provided callback to signal errors
  • import/export: Promisify Abiword and LibreOffice conversion
  • LibreOffice: Log conversion errors

@rhansen rhansen force-pushed the rhansen-export-errors branch from 8c93813 to 4cc1adc Compare March 17, 2021 20:17
@JohnMcLear
Copy link
Copy Markdown
Member

can we please modify the timeouts, not entirely remove them? I don't mind if they are longer but they are present for a purpose :)

@rhansen
Copy link
Copy Markdown
Member Author

rhansen commented Mar 17, 2021

Mocha has a default timeout so it won't hang or anything. I prefer to leave out timeouts unless there's a reason to change the default because they're hard to tune and make tests flaky.

@JohnMcLear
Copy link
Copy Markdown
Member

The test timeouts are for detecting if a change in an implementation slows down the functionality.

@rhansen rhansen force-pushed the rhansen-export-errors branch from 4cc1adc to 083071f Compare March 17, 2021 21:00
@rhansen
Copy link
Copy Markdown
Member Author

rhansen commented Mar 17, 2021

It's better to have purpose-built benchmarks for testing slowdows. Using timeouts for this purpose isn't particularly helpful because the tests will fail on slow machines even if the change did not slow things down, and the tests will pass on fast machines even if the change did slow things down.

@JohnMcLear
Copy link
Copy Markdown
Member

It's better to have purpose-built benchmarks for testing slowdows. Using timeouts for this purpose isn't particularly helpful because the tests will fail on slow machines even if the change did not slow things down, and the tests will pass on fast machines even if the change did slow things down.

While that may be the case can we please not make it part of the conversation of this PR to keep things simple and inline with the rest of the testing specs?

@rhansen rhansen force-pushed the rhansen-export-errors branch from 083071f to 2b4e48d Compare March 17, 2021 21:10
@rhansen rhansen force-pushed the rhansen-export-errors branch from 2b4e48d to 824c424 Compare March 17, 2021 21:13
@rhansen
Copy link
Copy Markdown
Member Author

rhansen commented Mar 17, 2021

Bumped the timeout to 1s.

@rhansen rhansen force-pushed the rhansen-export-errors branch 3 times, most recently from 0dc2aa7 to bbe8d3d Compare March 18, 2021 07:08
This avoids having two callbacks, which improves readability.
@rhansen rhansen force-pushed the rhansen-export-errors branch from bbe8d3d to 2a75b17 Compare March 18, 2021 07:20
@rhansen rhansen force-pushed the rhansen-export-errors branch from 2a75b17 to fd1164c Compare March 18, 2021 07:25
@rhansen rhansen changed the title export: Improve error handling import/export: Improve error handling, Promisify Mar 18, 2021
@rhansen rhansen changed the title import/export: Improve error handling, Promisify import/export: Improve error handling, promisify Mar 18, 2021
@rhansen
Copy link
Copy Markdown
Member Author

rhansen commented Mar 18, 2021

This is ready for review @JohnMcLear.

@rhansen rhansen requested a review from JohnMcLear March 18, 2021 09:00
@JohnMcLear JohnMcLear merged commit b4e1e93 into develop Mar 18, 2021
@JohnMcLear JohnMcLear deleted the rhansen-export-errors branch March 18, 2021 09:02
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