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

Feature retry cypher query #97

Merged
merged 8 commits into from
Jun 10, 2020
Merged

Feature retry cypher query #97

merged 8 commits into from
Jun 10, 2020

Conversation

huboneo
Copy link
Contributor

@huboneo huboneo commented Jun 9, 2020

Added runQuery to dbmss with a stepped retry over 20 sec

  • Also refactored PropertyFile to preserve whitespace
  • Also re-added "credential" flag to CLI cmd (for now)
  • Also added DbmsQueryError for failure records
  • Also re-enabled all skipped tests
  • Also added instance specific token salt

Screenshot 2020-06-09 at 16 51 22

huboneo added 2 commits June 9, 2020 21:58
- Also refactored PropertyFile to preserve whitespace
- Also re-added "credential" flag to CLI cmd (for now)
- Also added DbmsQueryError for failure records
@huboneo huboneo added bug Something isn't working enhancement New feature or request labels Jun 9, 2020
@huboneo huboneo requested review from nglgzz and jk05 June 9, 2020 20:13
@@ -24,6 +24,7 @@ ARGUMENTS
DBMS Name or ID of a Neo4j instance

OPTIONS
-c, --credentials=credentials (required)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to make this testable

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer we mock STDIN than adding a flag for credentials. Passing credentials as flags/arguments is not a good practice and I think we should avoid it as much as we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree. Do you have any example on doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just looked at the built-in mocking of STDIN that oclif provides. They don't close the stream after sending data, which makes it useless in our case.

An alternative to that would be mocking the passwordPrompt module using jest.

jest.mock('../../prompts', () => {
    return {
        passwordPrompt: (): Promise<string> => Promise.resolve(TestDbmss.DBMS_CREDENTIALS),
    };
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great suggestion, updated!

@huboneo
Copy link
Contributor Author

huboneo commented Jun 9, 2020

Re-enable skipped tests

@huboneo
Copy link
Contributor Author

huboneo commented Jun 9, 2020

@@ -4,7 +4,7 @@
"esModuleInterop": true,
"emitDecoratorMetadata": true,
"allowSyntheticDefaultImports": true,
"target": "es5",
"target": "es6",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nglgzz this is why instanceof ErrorAbstract wasnt working

Copy link
Contributor

@nglgzz nglgzz left a comment

Choose a reason for hiding this comment

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

Looks good to me

@huboneo huboneo merged commit d3407aa into neo4j-devtools:master Jun 10, 2020
@huboneo huboneo deleted the feature-retry-cypher-query branch June 10, 2020 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants