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

Sqlite adapter doesn't seem to like in-memory databases #3319

Closed
igorsantos07 opened this Issue Jul 6, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@igorsantos07
Contributor

igorsantos07 commented Jul 6, 2016

According to PHP's doc, we can use a simpler DSN for Sqlite in-memory database (sqlite::memory:). That seems neat for a short-living test, thus I gave it a shot.

Actually, it does work. But instead of using an actual memory memory connection, it created a :memory: file in my project root folder haha

I tested using that DSN from the CLI and it worked as expected, leaving no trace on the current folder.

Codeception 2.2.3, PHPUnit 5.4.6, PHP 5.6.23

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Jul 7, 2016

Member

Can in-memory database be shared by Codeception and your application?

Member

Naktibalda commented Jul 7, 2016

Can in-memory database be shared by Codeception and your application?

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jul 11, 2016

Member

Don't use memory database for testing.

  1. You can't debug it
  2. SQLite is probably differs from you primary application database so tests may be not relevant
  3. it will work for integration and functional tests only, but I think that the same way you can just set an ORM with nested transaction like we do for Doctrine, Eloquent, ActiveRecord of Yii, etc.

And yes, we don't support SQLite memory.

Member

DavertMik commented Jul 11, 2016

Don't use memory database for testing.

  1. You can't debug it
  2. SQLite is probably differs from you primary application database so tests may be not relevant
  3. it will work for integration and functional tests only, but I think that the same way you can just set an ORM with nested transaction like we do for Doctrine, Eloquent, ActiveRecord of Yii, etc.

And yes, we don't support SQLite memory.

@DavertMik DavertMik closed this Jul 11, 2016

@igorsantos07

This comment has been minimized.

Show comment
Hide comment
@igorsantos07

igorsantos07 Jul 12, 2016

Contributor

That seems more like a "we implemented a bug to disallow SQLite in-memory" instead of "we don't support it".
If that's the case, please catch the configuration and issue a proper warning, instead of allowing us to implement it and keep wondering why the hell it's creating a weirdly-named file instead of following the PDO behaviour.

Contributor

igorsantos07 commented Jul 12, 2016

That seems more like a "we implemented a bug to disallow SQLite in-memory" instead of "we don't support it".
If that's the case, please catch the configuration and issue a proper warning, instead of allowing us to implement it and keep wondering why the hell it's creating a weirdly-named file instead of following the PDO behaviour.

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Jul 13, 2016

Member
Member

Naktibalda commented Jul 13, 2016

@igorsantos07

This comment has been minimized.

Show comment
Hide comment
@igorsantos07

igorsantos07 Jul 15, 2016

Contributor

Thanks! Will do. I'm using SQLite currently to test a small Phalcon package that interacts with a new table, so it fits nicely. As soon as I have some tests passing I'll fix the SQLite implementation on sqlite::memory: and send a PR. Could this issue be reopened, @Naktibalda?

Contributor

igorsantos07 commented Jul 15, 2016

Thanks! Will do. I'm using SQLite currently to test a small Phalcon package that interacts with a new table, so it fits nicely. As soon as I have some tests passing I'll fix the SQLite implementation on sqlite::memory: and send a PR. Could this issue be reopened, @Naktibalda?

@Naktibalda Naktibalda reopened this Jul 15, 2016

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Jul 25, 2016

Member

@igorsantos07 any luck?
I can make it throw exception if user tries to set the file to ':memory:'.

Member

Naktibalda commented Jul 25, 2016

@igorsantos07 any luck?
I can make it throw exception if user tries to set the file to ':memory:'.

@Naktibalda Naktibalda added the Db label Jul 25, 2016

@igorsantos07

This comment has been minimized.

Show comment
Hide comment
@igorsantos07

igorsantos07 Jul 26, 2016

Contributor

I ended up leaving this for later and forgot it, @Naktibalda. Sorry!

So, I implemented it but I stumbled upon another issue (I saw this mentioned somewhere else...). Memory databases would only work if the connection was shared between Codeception and the actual tested codebase... And that doesn't seem feasible.

What does it mean?
Even if Codeception allows sqlite::memory:, and the tested codebase uses the same DSN, both will have different memory spaces and won't be talking with the same database.
Given that, I think an exception should be thrown if the DSN is sqlite::memory:, or a warning should be given on the output at least - so we wouldn't forbid usage, but warn this is probably not going to work.

To register, this is what I wrote:

//Codeception/Lib/Driver/Sqlite.php
    public function __construct($dsn, $user, $password)
    {
        $this->filename = substr($dsn, 7);
        if ($this->filename != ':memory:') { //special handling for in-memory databases
            $this->filename = Configuration::projectDir() . $this->filename;
        }
        $this->dsn = 'sqlite:' . $this->filename;
        parent::__construct($this->dsn, $user, $password);
    }
Contributor

igorsantos07 commented Jul 26, 2016

I ended up leaving this for later and forgot it, @Naktibalda. Sorry!

So, I implemented it but I stumbled upon another issue (I saw this mentioned somewhere else...). Memory databases would only work if the connection was shared between Codeception and the actual tested codebase... And that doesn't seem feasible.

What does it mean?
Even if Codeception allows sqlite::memory:, and the tested codebase uses the same DSN, both will have different memory spaces and won't be talking with the same database.
Given that, I think an exception should be thrown if the DSN is sqlite::memory:, or a warning should be given on the output at least - so we wouldn't forbid usage, but warn this is probably not going to work.

To register, this is what I wrote:

//Codeception/Lib/Driver/Sqlite.php
    public function __construct($dsn, $user, $password)
    {
        $this->filename = substr($dsn, 7);
        if ($this->filename != ':memory:') { //special handling for in-memory databases
            $this->filename = Configuration::projectDir() . $this->filename;
        }
        $this->dsn = 'sqlite:' . $this->filename;
        parent::__construct($this->dsn, $user, $password);
    }

Naktibalda added a commit to Naktibalda/Codeception that referenced this issue Jul 26, 2016

Naktibalda added a commit to Naktibalda/Codeception that referenced this issue Jul 27, 2016

@Naktibalda Naktibalda closed this Jul 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment