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

colored-terminal-output #290

Closed
wants to merge 9 commits into from

Conversation

abhakta-47
Copy link
Contributor

add colored console output as mentioned in #103

@abhakta-47
Copy link
Contributor Author

cmake and ctest are working fine thats why I didnt see this errors.
make build also working but make check giving error regarding some atomiticity related to gtest.

After further inspection, including private/target/stream.h in stumpless/target/stream.h is causing this error.
To define stumpless_set_severity_color fucntion signature

void 
stumpless_set_severity_color( struct stream_target *target,
                              const enum stumpless_severity severity,
                              const char *escape_code );

stream_target is needed from private/stream.h

@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Base: 92.88% // Head: 92.60% // Decreases project coverage by -0.27% ⚠️

Coverage data is based on head (500c2b3) compared to base (25b3bc5).
Patch coverage: 78.46% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           latest     #290      +/-   ##
==========================================
- Coverage   92.88%   92.60%   -0.28%     
==========================================
  Files          39       39              
  Lines        3358     3423      +65     
  Branches      422      440      +18     
==========================================
+ Hits         3119     3170      +51     
- Misses        160      169       +9     
- Partials       79       84       +5     
Impacted Files Coverage Δ
src/target/stream.c 81.98% <78.12%> (-6.02%) ⬇️
src/target.c 95.72% <100.00%> (+<0.01%) ⬆️
src/target/journald.c 93.24% <0.00%> (+0.09%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@goatshriek
Copy link
Owner

It looks like you are getting there! Changing the public-facing signature to use the generic target structure was the correct thing to do. Looks like some memory leaks are present, causing valgrind to fail: make sure you delete the severity array in the target destructor. Alternatively, since the severity array is of fixed size, consider allocating it in the struct itself instead of with dynamic memory calls.

Let me know once tests are added, and I will do a full review.

@abhakta-47
Copy link
Contributor Author

abhakta-47 commented Oct 31, 2022

_Atomic error is caused by including thread_safety header in cpp test files . But, private/target/stream.h have to be included in stream.cpp to use stream_target. private/target/stream.h intern includes threadsafety. Thats causing builds to fail.
What to do here?

@abhakta-47
Copy link
Contributor Author

TEST( SetSeverityColor, StdoutBasicArgs ){
    struct stumpless_target *target;
    const struct stumpless_error *error;
    struct stumpless_entry *basic_entry;
    struct stumpless_entry *result;
    int log_result;

    std::FILE* ofs = tmpfile();
  
    target = stumpless_open_stream_target( "stdout-test", ofs );

    stumpless_set_severity_color( target, STUMPLESS_SEVERITY_EMERG, "\033[31m" );
    
    basic_entry = stumpless_new_entry( STUMPLESS_FACILITY_USER,
                                     STUMPLESS_SEVERITY_EMERG,
                                     "stumpless-unit-test",
                                     "basic-entry",
                                     "basic test message" );
    log_result = stumpless_add_entry( target, basic_entry );
    
    char *written_msg = NULL;
    char *reset_code = NULL;
    size_t line1,line2;
    
    fseek(ofs, 0, SEEK_SET);
    
    getline( &written_msg, &line1, ofs);    
    getline( &reset_code, &line2, ofs);
    
    written_msg[5] = '\0';

    EXPECT_STREQ( written_msg, "\033[31m" );
    EXPECT_STREQ( reset_code, "\033[0m" );

  }

Is this approach ok ?

@goatshriek
Copy link
Owner

goatshriek commented Dec 4, 2022

Yes, that approach to testing looks fine!

I took a look at your changes, and it looks like things are closing in on finished. You appear to have some heap corruption issues, which may be stemming from the fact that new stream targets do not have their severity_colors array initialized to NULL pointers on creation. This is likely causing the failures you are seeing in the examples. Ideally, try to add a test that fails on this case, but in any event I believe that setting all severity color strings to NULL on creation (that is, in the new_stream_target function) should resolve that particular issue.

Keep it up!

@goatshriek
Copy link
Owner

Checking in to see if you intend to continue work on this, per the contribution guidelines.

@goatshriek goatshriek added the stale inactive issue or pull request label Mar 27, 2023
@goatshriek
Copy link
Owner

Closed as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale inactive issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants