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

Logger interface compatible with testing package #593

Open
drewwells opened this issue May 16, 2017 · 4 comments
Open

Logger interface compatible with testing package #593

drewwells opened this issue May 16, 2017 · 4 comments

Comments

@drewwells
Copy link

drewwells commented May 16, 2017

Issue description

It would be convenient if the Logger interface was compatible with the testing logger (Logf).

Example code

func TestMyTest(t *testing.T) {
// start mysql in a test with a container for instance
// test polls mysql waiting for the connection to come up
// meanwhile go-sql-driver/mysql dumps errors into the testing package
  someCodeSucceeds
// testing does not report log statements
}

Error log

mysql dumps messages into the terminal regardless of testing success or failure
[mysql] 2017/05/16 15:46:20 packets.go:33: unexpected EOF
[mysql] 2017/05/16 15:46:20 packets.go:33: unexpected EOF
[mysql] 2017/05/16 15:46:20 packets.go:33: unexpected EOF
[mysql] 2017/05/16 15:46:21 packets.go:33: unexpected EOF
[mysql] 2017/05/16 15:46:21 packets.go:33: unexpected EOF
[mysql] 2017/05/16 15:46:21 packets.go:33: unexpected EOF
[mysql] 2017/05/16 15:46:22 packets.go:33: unexpected EOF
[mysql] 2017/05/16 15:46:22 packets.go:33: unexpected EOF
[mysql] 2017/05/16 15:46:22 packets.go:33: unexpected EOF

Configuration

Driver version (or git SHA):
1.3.0
Go version: run go version in your console
1.8.1
Server version: E.g. MySQL 5.6, MariaDB 10.0.20
MySQL 5.6
Server OS: E.g. Debian 8.1 (Jessie), Windows 10
Oracle Linux 7

@drewwells
Copy link
Author

drewwells commented May 16, 2017

Probably the best way to implement this without breaking backwards compat is to change the Logger to an empty interface. Then use type inference to choose different loggers

type Logger interface {}

type printer interface {
  Print(v ...interface{})
}

type logfer interface {
  Logf(fmt string, args ...interface{})
}

type errLog struct {
  logger Logger
}

func (el errLog) Print(args ...interface{}) {
    switch el.logger.(type) {
       case logger:
         el.logger.Logf("%v", args...)
       case printer:
         el.logger.Print(args...)
    }
}

@drewwells
Copy link
Author

Workaround

type sqlLogger struct {
	*testing.T
}

func (s sqlLogger) Print(args ...interface{}) {
	s.T.Logf("%v", args...)
}

func TestA(t *testing.T) {
  mysql.SetLogger(sqlLogger{t})
}

@julienschmidt julienschmidt added this to the v1.4 milestone Oct 7, 2017
@julienschmidt julienschmidt modified the milestones: v1.4.0, v1.5.0 May 15, 2018
@sagikazarmark
Copy link

Although I cannot argue the convenience argument (mostly because it's subjective), I think what you labeled as "Workaround" is actually the solution. Interfaces are there to be defined by the consumer, not by other code we want it to be compatible with.

Changing to an empty interface is a bad idea IMHO, defeats the whole purpose of interfaces.

I would just leave things as they are. If that 6 extra LOC is really bothering you, I would just try to contribute it as an optional implementation.

@julienschmidt julienschmidt modified the milestones: v1.5.0, v1.6.0 Sep 7, 2019
@julienschmidt julienschmidt modified the milestones: v1.6.0, v1.7.0 Jun 1, 2020
@josesa
Copy link

josesa commented Nov 29, 2021

My pet peeve with the logger is that is Global on package level.
This means that is not possible to associate errors with contexts, requests or anything like it.

So wouldn't t be leaked and overwritten across parallel tests?

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

No branches or pull requests

5 participants