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

Move C3 code to separate class #72

Closed
wants to merge 2 commits into from

Conversation

marcovtwout
Copy link

This PR is a first step in improving #40
Instead of having a copy of c3.php in the project root it can be included from any location.

Basic usage:

require __DIR__ . '/vendor/autoload.php'; // If using Composer
//require __DIR__ . '/vendor/codeception/c3/c3.php'; // Or include it directly

$c3 = new Codeception\C3(__DIR__); // pass the directory that contains Codeception config files
$c3->run();

I only made the minimal changes to test this concept. Best to disable whitespace comparison when reviewing this PR.

There's more to be improved after this:

  • Add options to configure error log file and temp output directory
  • Make all __c3_-functions part of the class
  • Restructure remaining code blocks into functions
    But first I like to get feedback on the current changes and to check if there's any use cases I might have missed.

Basic usage:
```
require __DIR__ . '/vendor/codeception/c3/c3.php';
$c3 = new Codeception\C3(__DIR__);
$c3->run();
```
Copy link
Member

@Naktibalda Naktibalda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update Setup instructions in README and document how to use c3 with codecept.phar file.

) {
ini_set('memory_limit', $requiredMemory);
}
// Autoload Codeception classes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new major version that requires composer, so it would be better to make it depend on Codeception ^5.0 and remove this require block because functions will be loaded by Composer automatically.

Update: If codecept.phar file is used, we have to require codecept.phar/vendor/autoload.php file instead.

Copy link
Author

@marcovtwout marcovtwout May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not using Composer autoloader and directly including the file should still work, for example:

require __DIR__ . '/vendor/codeception/c3/c3.php';

$c3 = new Codeception\C3(__DIR__); // pass the directory that contains Codeception config files
$c3->run();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you mean that since this will be a new major version of c3, more legacy loading methods can be removed?

Also - I don't know when Codeception v5 will be out, but it's probably better if we can keep support for Codeception v4 as well?

define('C3_CODECOVERAGE_PROJECT_ROOT', Codeception\Configuration::projectDir());
define('C3_CODECOVERAGE_TESTNAME', $_SERVER['HTTP_X_CODECEPTION_CODECOVERAGE']);
// phpunit codecoverage shimming
if (!class_exists('PHP_CodeCoverage') and class_exists('SebastianBergmann\CodeCoverage\CodeCoverage')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These class aliases were used to support PHPUnit <5.4. They can be removed now.

class_alias('SebastianBergmann\CodeCoverage\Exception', 'PHP_CodeCoverage_Exception');
}
// phpunit version
if (!class_exists('PHPUnit_Runner_Version') && class_exists('PHPUnit\Runner\Version')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHPUnit 5 compatibility

@Naktibalda Naktibalda changed the base branch from 2.0 to main May 20, 2021 06:46
@DavertMik
Copy link
Member

Thanks @marcovtwout Looks really nice!

I like the idea but it must be tested very well before releasing. Unfortunately, we have only basic tests on Codeception Ci pipeline.
If we release this as a major version and c3 will be installed only from Composer I think this will be fine for most setups.
Anyway, I need to see that this works and maybe we could release it as beta and ask volunteers to test it.

@marcovtwout
Copy link
Author

@DavertMik Thanks for the feedback :)

Here's a preview with some of the improvements I mentioned already applied: https://github.com/marcovtwout/c3/pull/1/commits
And a quick link to the full file: https://github.com/marcovtwout/c3/blob/class_based_c3_improvements/c3.php

Since you are thinking about a new major version anyways, perhaps legacy compatibility parts can be removed as well?

@marcovtwout
Copy link
Author

marcovtwout commented Apr 4, 2023

@DavertMik @Naktibalda I revisited this PR after some time has passed. Seems to me this is still a good basis for further improvements.

I created a new PR basing the same changes on the actual version of main: #85

@marcovtwout marcovtwout closed this Apr 4, 2023
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.

3 participants