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

Athena/pr 352 debugging #1

Merged
merged 3 commits into from
Apr 8, 2018

Conversation

n-riesco
Copy link

@n-riesco n-riesco commented Apr 7, 2018

@shannonlal I got to fix the tests. I can't test the frontend. Please, let me know if these changes also fix the issue with the frontend.

A brief explanation of the changes:

  • 466ff00 At first, I didn't know the reason, but the tests were failing because without calling connect(conn), conn.athenaClient was undefined.
  • d9cf781 this is a minor change, it only fixes the sample credentials shown in the frontend.
  • fecd7ab in this commit, I made more changes than strictly necessary (e.g. I moved createAthenaClient back to drivers/athena.js while debugging, but it could perfectly stay in athena.js; sorry about that). Although the only problem in athena.js was tables(), I also tried to simplify the code by removing the creation of unnecessary Promise's and by using slice and map.

Please, let me know what you think of the changes.

n-riesco added 3 commits April 7, 2018 23:36
* All tests share the Athena connection.

* Added connect() test and ensured it runs first, so that
  connection.athenaClient is defined for the remaining tests.

* Defined helper function mockAthenaResponses(query, columnNames, rows).

* Fixed tables() test (tables() shouldn't return the column name
  `table_names`).
* Fixed tables() so that it doesn't return the column name table_names.

* Avoided the creation of unnecessary promises.

* Used slice() and map() to generate the response rows.
Copy link
Owner

@shannonlal shannonlal 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 this

@shannonlal shannonlal merged commit de4489b into shannonlal:master Apr 8, 2018
shannonlal pushed a commit that referenced this pull request Jul 28, 2018
* Add scheduler

* Add sort fns

* Move info to props and use defaultProps for mock

* Create scheduled query button

* Number bug fix

* Fix search and sort

* Use rowGetter

* Use selectors

* Remove stage 2 and  3 code

* fetch scheduled queries and add to store

* Add PropTypes

* Fix column resize issue

* save all queries in the store and filter by connection in Settings component

* Use react-click-outside, preview modal

* Create modal factory, lint

* Extract Modal, Row, Column components to own files

* add SQL syntax highlighting in schedule tab

* misc style fixes, add modal close button

* add tests for Scheduler, update Scheduler/Modal with fixes found testing

* Review Comments #1:

- Add comment
- Remove old code
- Add ms dev

* .js extensions

* Word wrap SQL preview

* Add Layout description comment

* Comment react-data-grid formatters

* Rename .react.js -> .jsx, import css files

* Fix indentation

* Better preview header spacing

* Bold SQL preview header

* Move new deps to devDependencies

* Make SchedulerPreview

*  test/app/components/Settings/Scheduler/Scheduler.test.js -> test/app/components/Settings/scheduler.test.jsx

* adapt scheduler test to additional sql element in query modal

* Fix scheduler-table styling issues

* add className prop type to SQL
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