Simple API: Fix bug where omitted parameters still override .sqlfluff #2563
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2563 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 163 163
Lines 11838 11844 +6
=========================================
+ Hits 11838 11844 +6
Continue to review full report at Codecov.
|
test/api/simple_test.py
Outdated
| def test__api__config_override(exclude_sqlfluff, kwargs, expected, tmpdir): | ||
| """Test that parameters to lint() override .sqlfluff correctly (or not).""" | ||
| tmp_path = pathlib.Path(str(tmpdir)) | ||
| config_path = tmp_path / ".sqlfluff" | ||
| with tmpdir.as_cwd(): | ||
| config_path.write_text( | ||
| """[sqlfluff] | ||
| exclude_rules = {}""".format( | ||
| exclude_sqlfluff | ||
| ) | ||
| ) | ||
| sql = "SELECT TRIM(name) AS name FROM some_table" | ||
| lint_results = sqlfluff.lint(sql, config_path=str(config_path), **kwargs) | ||
| assert expected == {"L027", "L029"}.intersection( | ||
| {lr["code"] for lr in lint_results} | ||
| ) |
There was a problem hiding this comment.
Rather than writing to a temp dir (which sounds flakey, prone to conflcits, and not very scalable) could we not just have a test .sqlfluff config file in our code in our tests dir and use that? I believe we use that model elsewhere.
There was a problem hiding this comment.
We could. I actually prefer this approach for "one-off" tests like this, because the directory structure for our test fixtures is pretty large. It's kind of the difference between 50 tests (directory: yes) versus 2 tests (directory: no).
tmpdir is a built-in feature of PyTest, which I've used on lots of projects. It works well and cleans things up nicely. 😊
We will likely never have more than a few tests like this, because the simple API is just a thin layer on top of SQLFluff, so most of the tests live elsewhere.
There was a problem hiding this comment.
This test is also a little unusual in that it is made up of both SQL and config files but also the parameters that will be passed to the simple API (note that expected is a Python dictionary that we pass to a function).
There was a problem hiding this comment.
I’m more concerned with two tests running at the same time, and getting some sort of race condition as the file is written by one process and read by another.
This could even be within the one run if using our parallel processing technique.
tmpdir is an OS-agnostic way of getting access to the temp directory location, but it doesn’t create a unique dir/file within that AFAIK to avoid that situation.
Maybe we don’t need a new file for this and can just use one of the existing ones to avoid bloating the code base?
There was a problem hiding this comment.
Ah. This isn't the same tmpdir you're thinking of. This is a purest "fixture" that creates a unique directory every time (it'll be a subdirectory of the "main" temp directory). So it's guaranteed safe from those kinds of race conditions.
You can see the docs here. PyTest has a bunch of nice built-in features like this that make life easier.
https://docs.pytest.org/en/6.2.x/tmpdir.html#the-tmpdir-fixture
There was a problem hiding this comment.
Ok, moved the .sqlfluff file to a directory in git -- no temp directories here. 🙏
Brief summary of the change made
Fixes #2560
Are there any other side effects of this change that we should be aware of?
Pull Request checklist
Please confirm you have completed any of the necessary steps below.
Included test cases to demonstrate any code changes, which may be one or more of the following:
.ymlrule test cases intest/fixtures/rules/std_rule_cases..sql/.ymlparser test cases intest/fixtures/dialects(note YML files can be auto generated withtox -e generate-fixture-yml).test/fixtures/linter/autofix.Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.