test(pytest): remove star imports to please ruff linter

[jenstroeger]

As discussed in comment #4377 (comment)

I’m not sure how to pick your PR #4363 (i.e. fork branch taimoorzaeem:test/pylint) as base, though. Short of forking your fork 🤔

[taimoorzaeem]

I’m not sure how to pick your PR #4363 (i.e. fork branch taimoorzaeem:test/pylint) as base, though. Short of forking your fork 🤔

Github CLI is an excellent tool for that. Please make sure all the tests pass locally. I see a lot failing tests. You have missed some imports.

[jenstroeger]

Please make sure all the tests pass locally.

I’m on macOS 13.7 (Ventura) and I struggle to run the tests locally. I was able to install and set up nix but running the tests fails as follows:

[nix-shell:~/opt/dev/postgrest]$ postgrest-test-io 
Configuration is affected by the following files:
- cabal.project
- cabal.project.freeze
Build profile: -w ghc-9.4.8 -O1
In order, the following will be built (use -v for more details):
 - postgrest-13.1 (lib) (first run)
 - postgrest-13.1 (exe:postgrest) (first run)
Preprocessing library for postgrest-13.1...
Building library for postgrest-13.1...

on the commandline: error: [-Werror]
    -split-sections is not useful on this platform since it always uses subsections via symbols. Ignoring.
Error: [Cabal-7125]
Failed to build postgrest-13.1 (which is required by exe:postgrest from postgrest-13.1).

I do have a local PG server running, I’m unsure if that’s useful for a nix environment though? Do you have any recommendations on how to resolve this issue?

I see a lot failing tests. You have missed some imports.

Actually that’s not imports, but fixtures. For example this test

postgrest/test/io/test_cli.py

Lines 97 to 120 in ea2c64d

	@pytest.mark.parametrize(
	"args,env,use_defaultenv,expect",
	map(itemgetter("args", "env", "use_defaultenv", "expect"), FIXTURES["cli"]),
	ids=map(itemgetter("name"), FIXTURES["cli"]),
	)
	def test_cli(args, env, use_defaultenv, expect, defaultenv):
	"""
	When PostgREST is run with <args> arguments and <env>/<defaultenv> environment variabales
	it should return. Exit code should be according to <expect_error>.
	"""
	# use --dump-config by default to make sure that the postgrest process will terminate for sure
	args = args or ["--dump-config"]
	env = env or {}
	if use_defaultenv:
	env = {**defaultenv, **env}
	if expect == "error":
	with pytest.raises(PostgrestError):
	print(cli(args, env=env))
	else:
	dump = cli(args, env=env).split("\n")
	if expect:
	assert expect in dump

declares a dependency on fixture defaultenv via its function arguments, but as the error here states:

E       fixture 'defaultenv' not found

that fixture wasn’t “found”. Previously (before this change) that fixture

postgrest/test/io/config.py

Lines 35 to 44 in ea2c64d

	@pytest.fixture
	def defaultenv(baseenv):
	"Default environment for PostgREST."
	return {
	**baseenv,
	"PGRST_DB_CONFIG": "true",
	"PGRST_LOG_LEVEL": "info",
	"PGRST_DB_POOL": "1",
	"PGRST_NOT_EXISTING": "should not break any tests",
	}

became available because it was imported through the star import. However, as the docs describe here and here I would recommend introducing a conftest.py file to share fixtures, instead of importing them.

To proceed with this change, perhaps we should

1. Explicitly import fixtures as before; and then
2. Introduce conftest.py to follow pytest conventions.

[taimoorzaeem]

 on the commandline: error: [-Werror]
    -split-sections is not useful on this platform since it always uses subsections via symbols. Ignoring.
Error: [Cabal-7125]
Failed to build postgrest-13.1 (which is required by exe:postgrest from postgrest-13.1).

Not sure, maybe try removing -split-sections flag from cabal.project file.

I do have a local PG server running, I’m unsure if that’s useful for a nix environment though? Do you have any recommendations on how to resolve this issue?

No, you don't need a local pg server, every dependency comes sandboxed in the nix environment.

However, as the docs describe here and here I would recommend introducing a conftest.py file to share fixtures, instead of importing them.

This is a great idea! Perhaps I would need to look into that because currently we have fixtures in multiple files and would require moving things around.

To finish this PR, I think the safest option would be to go with explicit imports for now. These aren't a lot of imports (4 or 5). Also, could you can add a big comment or TODO: ... above the imports about the details regarding conftest.py and how it can be used to share fixtures, include link, references etc.

[jenstroeger]

Not sure, maybe try removing -split-sections flag from cabal.project file.

Hmm ok, I’ll try that. Maybe I should ask in the discussion forum if there are any Mac users? I asked the community for tips in discussion #4384. Alternatively, I can work on my Linux server as well…

This is a great idea! Perhaps I would need to look into that because currently we have fixtures in multiple files and would require moving things around.

Yup, agreed. I can noodle on that after we fixed the lint?

To finish this PR […]

Alright, I do that in the morning. Do you mind if I fly blind for now (i.e. no local tests until I get the nix sorted) and use the CI for testing? That’ll make some noise, but hopefully not too much if I’m careful?

[taimoorzaeem]

Yup, agreed. I can noodle on that after we fixed the lint?

Yes, we can do later improvements after setting up the lint.

Alright, I do that in the morning. Do you mind if I fly blind for now (i.e. no local tests until I get the nix sorted) and use the CI for testing? That’ll make some noise, but hopefully not too much if I’m careful?

There is no hurry, but yes, I think that's ok if you are being careful.

[taimoorzaeem]

This isn't really working, because ruff still complains about these fixture imports as "not used". That's because ruff is a static analyzer and it can't detect pytest's dynamic fixtures injection 😿 .

As you mentioned earlier, I think it would be best to first move all fixtures to conftest.py. Another benefit I can see is that we can then have all the default rules, we wouldn't need to silence any rule, as we did in #4363.

Let me move things around and restructure overall first and then we can move forward with this PR.

[taimoorzaeem]

@jenstroeger Please go ahead and rebase on top of #4387.

[jenstroeger]

Please go ahead and rebase on top of #4387.

@taimoorzaeem done, still getting errors when running postgrest-test-big-schema locally, investigating.

[jenstroeger]

@taimoorzaeem all yours 🤓

[taimoorzaeem]

Looks good to merge 🚀. @jenstroeger Please change PR status from draft to ready.

This clears up these 3 lint errors from #4363 (comment):

268	F405	[x] undefined-local-with-import-star-usage
 12	F403	[x] undefined-local-with-import-star
  1	F401	[x] unused-import

[jenstroeger]

Please change PR status from draft to ready.

@taimoorzaeem donesir!