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

Add column type to a query result of InfluxDB #6505

Merged
merged 6 commits into from
Oct 8, 2023

Conversation

masayuki038
Copy link
Collaborator

@masayuki038 masayuki038 commented Oct 7, 2023

What type of PR is this?

  • Bug Fix

Description

Bug fix for #6179

I confirmed that it works fine to download these files (below) in a query page of Redash by manual test:

  • csv
  • tsv
  • excel

How is this tested?

  • Unit tests (pytest, jest)
  • Manually

Related Tickets & Documents

Closes #6179

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@justinclift justinclift added python Pull requests that update Python code Team Review Meets PR criteria, ready for full review labels Oct 7, 2023
@justinclift
Copy link
Member

@masayuki038 Thanks for this PR. 😄

It looks like it needs some adjustment to the formatting, in order to pass our CI checks:

tests/query_runner/test_influx_db.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tests/query_runner/test_influx_db.py:13:5: F401 [*] `redash.query_runner.influx_db.InfluxDB` imported but unused

@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

Merging #6505 (5cec158) into master (0f175b7) will increase coverage by 0.15%.
Report is 1 commits behind head on master.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master    #6505      +/-   ##
==========================================
+ Coverage   61.10%   61.26%   +0.15%     
==========================================
  Files         158      158              
  Lines       12883    12889       +6     
  Branches     1753     1755       +2     
==========================================
+ Hits         7872     7896      +24     
+ Misses       4765     4743      -22     
- Partials      246      250       +4     
Files Coverage Δ
redash/query_runner/influx_db.py 67.64% <78.57%> (+32.16%) ⬆️

@masayuki038
Copy link
Collaborator Author

@justinclift Thanks for your following up. I fixed the unused import issue and pushed the branch again. However, it failed...

Run docker-compose logs
  docker-compose logs
  shell: /usr/bin/bash -e {0}
  env:
    NODE_VERSION: 16.[2](https://github.com/getredash/redash/actions/runs/6440340439/job/17489465013?pr=6505#step:10:2)0.1
Couldn't find env file: /home/runner/work/redash/redash/.env
Error: Process completed with exit code 1.

Should I add .env to this branch ?

@guidopetri
Copy link
Contributor

Huh, that check should not have run. I'll take a look at it later. For now you can just ignore the failing check

raw_no_rows = {"series": [{"name": "typetest", "columns": ["time", "k1", "v1", "v2"], "values": []}]}


class TestTransformResult(TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind rewriting these as pytest tests? We want to move our codebase away from unittest.

guidopetri
guidopetri previously approved these changes Oct 7, 2023
Copy link
Contributor

@guidopetri guidopetri left a comment

Choose a reason for hiding this comment

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

This looks great, thanks so much @masayuki038 ! I left one more comment but it's a nit. I'm good with merging if you don't want to change the names, but I'll wait until tomorrow to merge in case you do :)

raw_no_rows = {"series": [{"name": "typetest", "columns": ["time", "k1", "v1", "v2"], "values": []}]}


def test_result():
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we rename the tests to be more descriptive? Maybe something like test_influxdb_result_types_with_rows and test_influxdb_result_types_with_no_rows_are_string?

@guidopetri guidopetri enabled auto-merge (squash) October 8, 2023 01:52
@guidopetri
Copy link
Contributor

Thanks so much @masayuki038 !

@guidopetri guidopetri merged commit 011f9ef into getredash:master Oct 8, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests that update Python code Team Review Meets PR criteria, ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when running CSV download with InfluxDB as data source
3 participants