Run python tests in Pyodide build

[cpcloud]

This PR now runs as much of the Python test suite under Pyodide as possible, to give some confidence about its functionality.

[cpcloud]

This is skipped even though technically it shouldn't be. Without the skipping this actually crashes the interpreter with a funky WASM stack trace about BigInt. I can dig around in a follow-up.

[carlopi]

Sure, this is a clear improvement on current status, tests are run and failure are mapped.

[cpcloud]

This should pass, happy to dig into it in a follow up.

[cpcloud]

Looks like timezones other than UTC don't seem to work, not sure why yet.

[cpcloud]

@Tishj Were there some spark-shim-api union tests added recently? Not sure why they are suddenly failing.

[cpcloud]

Nope, looks like the last changes there were 8 months ago

[cpcloud]

@carlopi Would you mind giving this a review?

[cpcloud]

Not sure why some multithread tests are failing, I only changed whether the test skips instead of checking first.

[carlopi]

Not sure why some multithread tests are failing, I only changed whether the test skips instead of checking first.

Failure might be related to the fact that import pyarrow as pa is now simply imported as pyarrow. I will check whether that's it, but seems plausible.

[cpcloud]

Oh, I can look into it

[cpcloud]

Hm, I don't see where pa is in use such that it would cause that failure

[cpcloud]

Ah, found it

[cpcloud]

Probably time someone added some basic static checks (other than formatting) to the python codebase 😅

[carlopi]

Main concern is non changing behaviour of Python regular tests, if those pass, I would mostly check with @Tishj on what he prefers for skipping the tests.

[carlopi]

Can you expand on this?

It seems to be working on https://duckdb.github.io/duckdb-pyodide/console by doing:

import duckdb
duckdb.execute("SELECT $2::date - $1::date", ['2024-01-02', '2024-03-04'])

unsure if the problem is in the testing (possibly due to handling of timezones / else) or this is actually broken.

[cpcloud]

Main concern is non changing behaviour of Python regular tests,

Which tests are you referring to?

I don't think I changed any actual material test function bodies, I'm just skipping based some tests known to not work on Emscripten builds, unless you consider the pyarrow imports, which @Tishj had already said was desirable (replacing the top level import with an importorskip call).

[carlopi]

I meant that once regular Python CI is green (unsure if there might be other unforeseen surprises like pyarrow -> pa, it's basically good to go.

[cpcloud]

I meant that once regular Python CI is green (unsure if there might be other unforeseen surprises like pyarrow -> pa, it's basically good to go.

Ah ok, great!

[carlopi]

This looks good to me! Thanks

[carlopi]

I would say if @Tishj can give a second look to the tests, but if it makes sense to him this looks great.

[Tishj]

I am not entirely confident this will be unique for every test

I have more faith in this fixture, do you mind using this?

@pytest.fixture(scope="function")
def temp_file_name(request, tmp_path_factory):
    return str(tmp_path_factory.mktemp(request.function.__name__, numbered=True) / 'file.csv')

tmp_path_factory creates unique paths, and numbers them, + for good measure I included the name of the test as another unique token

We are creating more than one files in certain places, perhaps this fixture should return a generator then

[cpcloud]

I am not entirely confident this will be unique for every test

The pytest documentation for tmp_path says almost verbatim exactly that, which is why I chose it :)

[cpcloud]

The implementation of tmp_path is very similar to your proposed implementation (I removed the docstring and clean up code that follows the yield):

def _mk_tmp(request: FixtureRequest, factory: TempPathFactory) -> Path:
    name = request.node.name
    name = re.sub(r"[\W]", "_", name)
    MAXVAL = 30
    name = name[:MAXVAL]
    return factory.mktemp(name, numbered=True)


@fixture
def tmp_path(
    request: FixtureRequest, tmp_path_factory: TempPathFactory
) -> Generator[Path, None, None]:
    path = _mk_tmp(request, tmp_path_factory)
    yield path

[Tishj]

Thanks for the changes and making it as less invasive as possible, looks good to me 👍

[Mytherin]

Thanks!