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

Testing RF to share among extractors #114

Closed
wants to merge 34 commits into from
Closed

Conversation

tomasfejfar
Copy link
Contributor

@tomasfejfar tomasfejfar commented Mar 26, 2019

@tomasfejfar tomasfejfar marked this pull request as ready for review April 16, 2019 06:51
@tomasfejfar
Copy link
Contributor Author

What do you think @pivnicek @MiroCillik ? It's a PoC.

I don't like how much code you need to write to implement the create table/insert rows. But I can't think of any other way to do it reliably. The table creation could be local in the test, but that would not work if something new appears in common test. You'd need to know how to change it and it will be pain.

I think we can supply AbstractPdoDataLoader as well, that will save most of the work for PDO based extractors. And maybe AbstractOdbcDataLoader for almost all the rest. And what will remain will be few exceptions where we need to do the work.

@MiroCillik
Copy link
Member

I think this is the right way to go. The AbstractPdoDataLoader and the ODBC one should cover most of our extractors except Oracle :)

Do you plan or think of writing the actual Tests similarly? I mean to have some abstract classes with test methods in the common bundle, which will define what to test and these will be extended in the "child" extractors?

@tomasfejfar
Copy link
Contributor Author

That's pretty much the idea.

I actually forgot to change the CommonExtractor test. This should be the first step. Keep the current tests, just write them above the AbstractTestCase class. It will practically only be implementing the getDataLoader method. I'll commit momentarily.

@tomasfejfar
Copy link
Contributor Author

I'm afraid that this is going to be a fuckup :(

I did quite a lot of work on it - most of it was to validate the idea though :(

  • first - I reimplemented the current "common" extractor tests over the abstract test. That worked ✔️
  • second - I wanted to validate my abstraction with different extractor and tried to implement it in MSSQL ex. I approached it so, that I started hacking the failing tests one-by-one . It worked for simple test - extracting the table by name. I got that to work with few little tweaks in the new db-ex-common test code and a hack - hardcoded schema. But it all got gradually more complicated now when I approached the next tests. Some of them have assumptions about the underlying database - like that there is an autoincrement field (e.g. id can be null), or that a query can select from table without mentioning schema (not the case in mssql, or at least not easily).

It starts to feel like too much hacking to get it to work and that's just one extractor - what about Firebase or Terradata - that would probably be too much hassle :( This may need to be approached from a completely different angle :(

Alternatives that come to mind would be just a TestCase with tests like this:

public function testSimpleExtract() {
  $this->testSimpleEnsureTableWithAutoincrementColumnExists();
  $this->testSimpleInsertRowIntoTheTable();
  $this->testSimpleAssertThatExtractorProducesOneRowInIncrementalMode();
  sleep(1);
  $this->testSimpleInsertRowIntoTheTable();
  $this->testSimpleAssertThatExtractorProducesOnlySecondRowInIncrementalMode();
}

But that still might be too constraining :( Maybe only testExtractTableSimple, testExtractUsingQuery, testIncrementalUsingNumericIndex, testIncrementalUsingDateField without anything else.

Anyway, that would just make you implement it yourself. But there is no guarantee that the test would actually test what it's supposed to - e.g. that when we found the issue with datetime incremental extract - that you need to extract all INCLUDING any that were created during the "lastExtraction" second - there would be no guarantee that the other extractor's tests wouldn't manifest the same problem.

But that may be the best thing we can come up with. Maybe it could come with prepared CSV files that would be loaded and ten checked against the expected. But even that is PITA for some of the scenarios - like when the CSV contains only the data without ID, but there is autoincrement column in mysql. That wouldn't then work with mssql.

And then I learned that oracle extractor is rewritten to Java. So that even if we implemented datadir tests that are almost opaque it still wouldn't work :( The tests would need to be on the docker level. That would be a huge feat and wouldn't be worth the trouble IMHO.

I lost faith that the db-ex can actually be tested with one test code base :(

Could you please @pivnicek and @MiroCillik check it out and tell me what you think.

@tomasfejfar
Copy link
Contributor Author

Ukázalo se, že tohle není dobrá cesta. Minimálně do doby, než budeme z nějakého jiného důvodu potřebovat common abstrakci nad všemi různými databázemi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants