Allow reading of already queued lines even after pipe has closed

korjaa commented 5 years ago

Status

WORK-IN-PROGRESS

Migrations

NO

Description

Even if pipe is closed and it sets state to error the readline should read all the already received and queued lines. Otherwise in some cases last traces / output from process is lost.

Related PRs

-

Todos

[x] Tests
[x] Documentation

Deploy Notes

-

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

groan

Impacted Areas in Application

List components, applications or use-cases that this PR will affect:

Tests with "process" DUTs

korjaa commented 5 years ago

Found a nice icetea patch from @kuggenhoffen :)

korjaa commented 5 years ago

Don't merge yet, I'll add one more fix for catching last bytes if DUT dies quickly.

korjaa commented 5 years ago

That should do it.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 768

0 of 8 (0.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.03%) to 70.514%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
icetea_lib/GenericProcess.py	0	8	0.0%
<!--	Total:	0	8	0.0%	-->

Files with Coverage Reduction	New Missed Lines	%
icetea_lib/GenericProcess.py	1	16.0%
<!--	Total:	1		-->

Totals
Change from base Build 760:	-0.03%
Covered Lines:	4998
Relevant Lines:	7088

💛 - Coveralls

kuggenhoffen commented 5 years ago

@korjaa It would be good idea to test if the same issue happens in OS X with the kQueue implementation as well.

jonikula commented 5 years ago

@korjaa are you going to test as @kuggenhoffen suggested or should I merge this?

korjaa commented 5 years ago

This can be used to verify if that this patch works.

import time
from icetea_lib.bench import Bench
from icetea_lib.bench import TestStepFail

class Testcase(Bench):
    def __init__(self):
        Bench.__init__(
            self,
            name="verify_quick_exit",
            type="smoke",
            requirements={
                "duts": {
                    '*': {
                        "count": 1,
                        "type": "process"
                        },
                    1: {
                        "application": {
                            "bin": "/bin/echo",
                            "bin_args": ["If this is found, the test passed"],
                            "init_cli_cmds": [],
                            "post_cli_cmds": []
                        }
                    }
                }
            }
        )

    def setup(self):
        pass

    def case(self):
        time_start = time.time()
        result = False
        while (time.time() - time_start) < 10.0 and result == False:
            result = self.verify_trace(0, "If this is found, the test passed", False)
            time.sleep(1.0)
        if result == False:
            raise TestStepFail("Didn't get trace")

    def teardown(self):
        pass

korjaa commented 5 years ago

@SeppoTakalo could you check the above testcase, I don't have OS X with kQueue. If ran against master, it should FAIL. If ran against this PR, it should PASS.

jonikula commented 5 years ago

Any news on this? @korjaa @SeppoTakalo

korjaa commented 5 years ago

~Even with these changes, I've seen couple of times traces been lost.~ Noticed my icetea had falled back to old version, cannot reproduce the issue anymore with the latest version.

jonikula commented 5 years ago

I'm pushing this backwards so I can make the v1.1.1 release today hopefully.

korjaa commented 5 years ago

@jupe Any chance you could test this on Mac?

jonikula commented 5 years ago

I can merge this if I'm given the ok.

korjaa commented 5 years ago

New included test_genericprocess.py test can be executed with

coverage run -m unittest discover -s test -p test_genericprocess.py

korjaa commented 5 years ago

Fixing unittests, windows returning some weird return code.

korjaa commented 5 years ago

Rebased and squashed.

jupe commented 5 years ago

ci/circleci: test-3.5 is broken at the moment

ARMmbed / icetea