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

Feature: Adding StreamFilterTrait #6112

Merged
merged 6 commits into from
Jun 15, 2022

Conversation

iRedds
Copy link
Collaborator

@iRedds iRedds commented Jun 11, 2022

Description
StreamFilterTrait trait, which correctly adds and removes filters for capturing stream output, since the current implementation in the tests is not correct enough in my opinion.

Overview of methods

  • StreamFilterTrait::registerStreamFilterClass() Registering a filter to capture streams.
  • StreamFilterTrait::appendStreamOutputFilter() Adding a filter to the output stream.
  • StreamFilterTrait::appendStreamErrorFilter() Adding a filter to the error stream.
  • StreamFilterTrait::removeStreamOutputFilter() Removing the filter from the output stream.
  • StreamFilterTrait::removeStreamErrorFilter() Removing the filter from the error stream.
  • StreamFilterTrait::getStreamFilterBuffer() Get the captured data from the buffer.
  • StreamFilterTrait::resetStreamFilterBuffer() Reset captured data.
-        CITestStreamFilter::$buffer = '';
-
-        $this->streamFilter = stream_filter_append(STDOUT, 'CITestStreamFilter');
-        $this->streamFilter = stream_filter_append(STDERR, 'CITestStreamFilter');
+        $this->registerStreamFilterClass()
+            ->appendStreamOutputFilter()
+            ->appendStreamErrorFilter();

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

iRedds added a commit to iRedds/CodeIgniter4 that referenced this pull request Jun 11, 2022
@kenjis kenjis added 4.3 enhancement PRs that improve existing functionalities labels Jun 11, 2022
@kenjis
Copy link
Member

kenjis commented Jun 11, 2022

Can you rebase to remove unrelated commits?

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 11, 2022

I may be doing something wrong, but the PR has already been rebased.

@kenjis
Copy link
Member

kenjis commented Jun 11, 2022

Did you do this?

git fetch upstream
git rebase upstream/4.3

@iRedds You're right. This brach has been rebased.
Is this GitHub problem?

@kenjis kenjis changed the base branch from 4.3 to develop June 11, 2022 14:41
@kenjis kenjis changed the base branch from develop to 4.3 June 11, 2022 14:41
@kenjis
Copy link
Member

kenjis commented Jun 11, 2022

Okay, fixed.

@kenjis
Copy link
Member

kenjis commented Jun 11, 2022

Probably you created this PR branch from develop and it was ahead than 4.3 at that time.
So unrelated commits were included in this PR.

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 11, 2022

I've rebased onto upstream/develop.
As always. As in the manual.

Now the new PR should be based on 4.3 and rebased on upstream/4.3?

@kenjis
Copy link
Member

kenjis commented Jun 11, 2022

Sorry, the manual is not updated yet.

Now we have two development branches, develop and 4.3.

  • develop is for 4.2.x, that is bug fixe only.
  • 4.3 is for 4.3.x.

This is so that bug fix versions can be released quickly.

Now the new PR should be based on 4.3 and rebased on upstream/4.3?

The bug fix only PRs are based on develop and rebased on upstream/develop.
PRs with any enhancements are based on 4.3and rebased on upstream/4.3.

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 11, 2022

You came up with something complicated )

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

This seems handy!

My suggestion: Can we make StreamOutput to OutputStream and StreamError to ErrorStream, i.e. appendStreamOutputFilter vs appendOutputStreamFilter? In my opinion, that will make the methods sound more "natural" and would make the methods descriptive of their names (e.g., appendOutputStreamFilter is "append filter to the output stream").

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 11, 2022

@paulbalandan I agree. It really does sound better.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

This is great! I always end up copying the framework test methods into projects when I need buffer testing.

My only thought: should automate stream registration when using the trait? Or stream removal? Or both?

https://codeigniter4.github.io/userguide/testing/overview.html#traits

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 12, 2022

@paulbalandan already added. f67d745

I'm not good at github actions, but if I'm right, the 4.3 branch won't show up here. Moreover, it is still PR.

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 12, 2022

@MGatner If it's only about registering a filter class, then I think it's a good idea.
But regarding gluing the filter to the stream. I have doubts. So I'd like to hear more opinions on this.

Up: the idea is good, but if the setUp() method is overridden in the test class, then the methods for registering a filter, adding a filter to streams will not be called. And most tests override the setUp() and tearDown() methods without calling the parent.

protected function setUp(): void
{
CITestStreamFilter::$buffer = '';
$this->streamFilter = stream_filter_append(STDOUT, 'CITestStreamFilter');
$this->streamFilter = stream_filter_append(STDERR, 'CITestStreamFilter');
}

So I want to hear more opinions on your proposal as a whole.

@kenjis
Copy link
Member

kenjis commented Jun 13, 2022

And most tests override the setUp() and tearDown() methods without calling the parent.

I think it is a bug that does not call parent.

Are there use cases that we don't want to append/remove the stream filters automatically?

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 13, 2022

@kenjis I can't think of a situation yet that would cause problems. That's why I asked for an opinion.

@MGatner
Copy link
Member

MGatner commented Jun 13, 2022

I guess the docs aren't very clear, probably my fault. But traits can provide their own methods that will be called automatically: setUpStreamFilterTrait(), tearDownStreamFilterTrait().

I think since most cases if a developer is using the trait they want this baseline we should consider making that happen without intervention.

Edit: see callTraitMethods() in system/Test/CIUnitTestCase.php

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 13, 2022

@MGatner Thanks for the clarification, but I initially understood your idea of trait preconfiguration.
I will try to implement the changes you suggested.

@kenjis
Copy link
Member

kenjis commented Jun 14, 2022

Please add:

--- a/user_guide_src/source/changelogs/index.rst
+++ b/user_guide_src/source/changelogs/index.rst
@@ -12,6 +12,7 @@ See all the changes.
 .. toctree::
     :titlesonly:
 
+    v4.3.0
     v4.2.1
     v4.2.0
     v4.1.9

@kenjis
Copy link
Member

kenjis commented Jun 14, 2022

Sorry for a comment after a comment.
Probably the previous one is the last.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Kenjis making you work for it 😂
This is looking great and I'm excited about the feature. I'm good with it whenever you all think it is ready.

@iRedds iRedds force-pushed the feature/streamfilter-trait branch from dc41e45 to 50c3a14 Compare June 15, 2022 05:20
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

LGTM!

@kenjis kenjis merged commit b0aa4dd into codeigniter4:4.3 Jun 15, 2022
@kenjis
Copy link
Member

kenjis commented Jun 15, 2022

@iRedds Thank you!
This is a great improvement for CLI testing.

@iRedds iRedds deleted the feature/streamfilter-trait branch June 15, 2022 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants